* [PATCH v2 0/5] phy: cpcap-usb: improve charger detection and export cable state
From: Ivaylo Dimitrov @ 2026-07-05 7:58 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Linus Walleij, Bartosz Golaszewski
Cc: linux-phy, devicetree, linux-kernel, linux-omap, linux-gpio,
Ivaylo Dimitrov
The Motorola CPCAP USB PHY contains the hardware state machine used for
USB cable detection. Besides distinguishing USB peripheral and host
connections, it can also detect dedicated charging ports (DCP).
This series starts with a fix to prevent spurious SysRq events when
switching between USB and UART modes. It then adds DCP detection support
to the CPCAP USB PHY, updates the Device Tree binding and corresponding
mapphone Device Tree to use the charger detection interrupt, and finally
exports the detected cable state through the Extcon framework.
The existing driver already interprets the CPCAP USB detection state
machine to determine the attached cable type. This series extends that
logic to distinguish DCP connections and exposes the detected cable state
through Extcon using a standard kernel interface. It also makes the idle
UART mode optional, allowing the PHY to remain in its default USB
detection configuration unless UART support is explicitly requested.
The series has been tested on Motorola Droid 4 hardware.
Changes in v2:
- add chrg_det interrupt instead of replacing se1
- Fix remove() race by cancelling detect_work before hardware teardown
- Restore SysRq state on all error paths
- Introduce CPCAP_UNKNOWN initial mode
- Snapshot enable_uart module parameter
Ivaylo Dimitrov (5):
phy: cpcap-usb: Prevent line glitches from triggering sysrq
dt-bindings: phy: motorola,cpcap-usb: add chrg_det interrupt
ARM: dts: ti: cpcap-mapphone: use charger detection interrupt for
CPCAP USB PHY
phy: cpcap-usb: add DCP detection and make UART idle mode optional
phy: cpcap-usb: add extcon support
.../bindings/phy/motorola,cpcap-usb-phy.yaml | 6 +-
.../dts/ti/omap/motorola-cpcap-mapphone.dtsi | 6 +-
drivers/phy/motorola/phy-cpcap-usb.c | 334 +++++++++++++++---
3 files changed, 290 insertions(+), 56 deletions(-)
--
2.39.5
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v2 2/2] phy: spacemit: Add USB3/PCIe comb PHY driver for Spacemit K3
From: Inochi Amaoto @ 2026-07-05 1:59 UTC (permalink / raw)
To: sashiko-reviews, Inochi Amaoto
Cc: linux-phy, vkoul, neil.armstrong, olteanv, robh, conor+dt,
devicetree
In-Reply-To: <20260703021929.15FDE1F000E9@smtp.kernel.org>
On Fri, Jul 03, 2026 at 02:19:28AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Hardware register clobbering due to missing read-modify-write operations in PHY initialization.
> --
>
> commit c71cb5c4ebfcb4fc5aff5c18a7769949e15dae55
> Author: Inochi Amaoto <inochiama@gmail.com>
>
> phy: spacemit: Add USB3/PCIe comb PHY driver for Spacemit K3
>
> Adds a USB3/PCIe comb PHY driver for Spacemit K3. The comb PHY requires
> configuring a syscon device for the correct mux configuration and needs
> calibration before usage.
>
> > diff --git a/drivers/phy/spacemit/phy-k3-common.c b/drivers/phy/spacemit/phy-k3-common.c
> > new file mode 100644
> > index 0000000000000..0b289e63db999
> > --- /dev/null
> > +++ b/drivers/phy/spacemit/phy-k3-common.c
>
> [ ... ]
>
> > + /* Power down 100MHz refclk buffer */
> > + val = readl(base + PHY_PU_CK_REG);
> > + val = u32_replace_bits(val, 0, PHY_PU_REFCLK_100);
> > + writel(val, base + PHY_PU_CK_REG);
> > +
> > + /* Program PLL REG1 configure the SSC */
> > + val = FIELD_PREP(PHY_PLL_SSC_MODE, PHY_PLL_SSC_MODE_DOWN_SPREAD1) |
> > + FIELD_PREP(PHY_PLL_SSC_DEP_SEL, PHY_PLL_SSC_5000PPM) |
> > + FIELD_PREP(PHY_PLL_FREF_SEL, PHY_PLL_FREF_24M);
> > + writel(val, base + PHY_PLL_REG1);
>
> [Severity: High]
> Does this completely overwrite PHY_PLL_REG1 and zero out unspecified bits?
>
No, All the field are filled, so no data is lost.
> Unlike the PCIe initialization path which uses a read-modify-write for this
> register, this direct writel() in k3_usb3phy_init_single() clobbers the
> entire register. Could this cause important default settings or calibration
> values to be lost?
>
> [ ... ]
>
> > + for (i = 0; i < lg->data->lanes; i++) {
> > + void __iomem *lane_base = lg->base + lg->data->offsets[i];
> > +
> > + val = readl(lane_base + PCIE_RX_REG1);
> > + val = u32_replace_bits(val, 0, 0x3);
> > + writel(val, lane_base + PCIE_RX_REG1);
> > + }
> > +
> > + val = readl(phy_base + PHY_PLL_REG2);
> > + val |= BIT(20);
> > + writel(val, phy_base + PHY_PLL_REG2);
> > +
> > + /* The write is needed as clock requires renegotiation */
> > + val = FIELD_PREP(PCIE_RX_REFCLK_MODE, PCIE_RX_REFCLK_MODE_DRIVER) |
> > + PCIE_RX_SEL_TRI_CODE |
> > + FIELD_PREP(PCIE_RX_LEGACY, PCIE_RX_LEGACY_DEFAULT);
> > + writel(val, phy_base + PCIE_RX_REG1);
>
> [Severity: High]
> Does this clobber the entire PCIE_RX_REG1 register for lane 0?
>
This also serves as the whole clock. If the phy has two lanes, they share some
data and the data is store at lane 0.
> The preceding loop performs a read-modify-write on PCIE_RX_REG1 for all lanes,
> but this direct writel() on phy_base (which points to lane 0) invalidates
> that previous work and zeroes out any bits not explicitly set here.
>
> Should this also use a read-modify-write operation to preserve the register
> state?
>
No, it is not needed, as all the field are filled or it should be zero.
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260703021024.495433-1-inochiama@gmail.com?part=2
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v2 2/2] iio: inkern: Use namespaced exports
From: Jonathan Cameron @ 2026-07-05 1:07 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Romain Gantois, MyungJoo Ham, Chanwoo Choi, Guenter Roeck,
Peter Rosin, David Lechner, Nuno Sá, Andy Shevchenko,
Lars-Peter Clausen, Michael Hennerich, Mariel Tinaco, Kevin Tsai,
Linus Walleij, Eugen Hristev, Vinod Koul, Kishon Vijay Abraham I,
Sebastian Reichel, Chen-Yu Tsai, Hans de Goede,
Support Opensource, Paul Cercueil, Iskren Chernev,
Krzysztof Kozlowski, Marek Szyprowski, Matheus Castello,
Saravanan Sekar, Matthias Brugger, AngeloGioacchino Del Regno,
Casey Connolly, Pali Rohár, Orson Zhai, Baolin Wang,
Chunyan Zhang, Amit Kucheria, Thara Gopinath, Rafael J. Wysocki,
Daniel Lezcano, Zhang Rui, Lukasz Luba, Claudiu Beznea,
Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Sylwester Nawrocki, Olivier Moysan, Arnaud Pouliquen,
Maxime Coquelin, Alexandre Torgue, Thomas Petazzoni, linux-kernel,
linux-hwmon, linux-iio, linux-input, linux-phy, linux-pm,
linux-mips, linux-mediatek, linux-arm-msm, linux-sound,
linux-stm32, Sebastian Reichel, Andy Shevchenko
In-Reply-To: <20260603182052.7d23c067@jic23-huawei>
On Wed, 3 Jun 2026 18:20:52 +0100
Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun, 22 Mar 2026 15:24:21 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> > On Tue, Dec 09, 2025 at 09:25:56AM +0100, Romain Gantois wrote:
> > > Use namespaced exports for IIO consumer API functions.
> > >
> > > This will make it easier to manage the IIO export surface. Consumer drivers
> > > will only be provided access to a specific set of functions, thereby
> > > restricting usage of internal IIO functions by other parts of the kernel.
> > >
> > > This change cannot be split into several parts without breaking
> > > bisectability, thus all of the affected drivers are modified at once.
> > >
> > > Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com> # for power-supply
> > > Acked-by: Guenter Roeck <linux@roeck-us.net>
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > Signed-off-by: Romain Gantois <romain.gantois@bootlin.com>
> >
> > For input:
> >
> > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >
> > Thanks.
> >
>
> For anyone wondering what happened to this... I forgot to apply this at the
> beginning of the cycle and by the time I remembered we had too much queued up
> so it would have been messy to do an immutable branch. Anyhow, I plan to
> sort this at start of next cycle.
Immutable branch created as: iio-inkern-namespace-ib based on v7.2-rc1
on https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
I've merged it into the testing branch of iio.git so if anyone else does
need to merge this to avoid conflicts, perhaps wait a day or two until
I've pushed it out as togreg for linux-next to pick up.
Thanks,
Jonathan
>
> Jonathan
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 5/5] phy: cpcap-usb: add extcon support
From: sashiko-bot @ 2026-07-04 7:51 UTC (permalink / raw)
To: Ivaylo Dimitrov
Cc: conor+dt, devicetree, neil.armstrong, vkoul, linux-phy, robh,
olteanv
In-Reply-To: <20260704073843.1750458-6-ivo.g.dimitrov.75@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] The EXTCON_USB state is updated using `extcon_set_state` without a corresponding `extcon_set_state_sync` or `extcon_sync`, preventing registered listeners from receiving state change notifications.
- [Medium] Extcon state can become permanently desynchronized from the hardware/software state when regmap updates fail.
Pre-existing issues:
- [High] The driver's remove path races with the devres-managed threaded IRQ handler, leading to a Use-After-Free of the driver data structure.
--
commit 5e21d209058ee93304da0e1964b84339cf8709c2
Author: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
phy: cpcap-usb: add extcon support
Register an Extcon device and report the detected cable state.
The driver already determines the type of cable attached during USB cable
detection. Export the detected state through the Extcon framework so
other drivers can consume it using a standard kernel interface.
> diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
> index 3be2b3afaaf4d..8978a95c1c199 100644
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c
> @@ -123,6 +124,7 @@ struct cpcap_phy_ddata {
> struct regmap *reg;
> struct device *dev;
> struct usb_phy phy;
> + struct extcon_dev *edev;
> struct delayed_work detect_work;
[Severity: High]
This is a pre-existing issue, but does the remove path race with the
devres-managed threaded IRQ handler, leading to a use-after-free of this
driver data structure?
If the devres-managed IRQ thread cpcap_phy_irq_thread() fires and reads
ddata->active as 1, but is preempted before calling schedule_delayed_work():
And then the driver remove path cpcap_usb_phy_remove() executes, sets
ddata->active to 0, cancels any pending work, and returns.
Devres cleanup then begins, calling devm_irq_release() -> free_irq() ->
synchronize_irq(), which blocks waiting for the preempted IRQ thread.
When the IRQ thread resumes, it successfully queues the delayed work and
returns.
Once devm_irq_release() finishes, devres proceeds to free ddata.
The delayed work timer expires and executes cpcap_usb_detect() on the freed
ddata memory.
> [ ... ]
> @@ -226,6 +239,43 @@ static void cpcap_usb_try_musb_mailbox(struct cpcap_phy_ddata *ddata,
> [ ... ]
> +static void cpcap_usb_set_extcon(struct cpcap_phy_ddata *ddata)
> +{
> [ ... ]
> + if (ddata->cable == EXTCON_CHG_USB_SDP)
> + extcon_set_state(ddata->edev, EXTCON_USB, false);
> + else if (cable == EXTCON_CHG_USB_SDP)
> + extcon_set_state(ddata->edev, EXTCON_USB, true);
[Severity: High]
Does this prevent registered listeners from receiving state change
notifications for EXTCON_USB?
The EXTCON_USB state is updated here using extcon_set_state() without a
corresponding extcon_set_state_sync() or extcon_sync(). When the driver
changes the state to or from CPCAP_USB (which maps to EXTCON_CHG_USB_SDP),
the subsequent extcon_set_state_sync() calls only target the charger cable
ID and its specific notifier chain.
The notifier chain for EXTCON_USB might never be fired.
> +
> + if (ddata->cable != EXTCON_NONE)
> + extcon_set_state_sync(ddata->edev, ddata->cable, false);
> +
> + if (cable != EXTCON_NONE)
> + extcon_set_state_sync(ddata->edev, cable, true);
> +
> + ddata->cable = cable;
> +}
> [ ... ]
> @@ -356,6 +411,8 @@ static void cpcap_usb_detect(struct work_struct *work)
> [ ... ]
> if (ddata->mode == CPCAP_IDLE)
> return;
>
> ddata->mode = CPCAP_IDLE;
> cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_OFF);
>
> error = cpcap_usb_set_idle_mode(ddata);
> if (error)
> goto out_err;
>
> + cpcap_usb_set_extcon(ddata);
> +
> return;
>
> out_err:
[Severity: Medium]
Can the extcon state become permanently desynchronized from the hardware and
software state if a regmap update fails?
Earlier in cpcap_usb_detect(), the internal software state ddata->mode is
updated prior to hardware configuration. If the hardware register update
fails, the code branches to out_err and skips calling
cpcap_usb_set_extcon().
Because ddata->mode is already modified, subsequent executions of
cpcap_usb_detect() will take the early return path and will never retry
updating the extcon state, leaving it stale indefinitely.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260704073843.1750458-1-ivo.g.dimitrov.75@gmail.com?part=5
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 2/5] phy: cpcap-usb: add DCP detection and make UART idle mode optional
From: sashiko-bot @ 2026-07-04 7:49 UTC (permalink / raw)
To: Ivaylo Dimitrov
Cc: robh, conor+dt, devicetree, neil.armstrong, olteanv, vkoul,
linux-phy
In-Reply-To: <20260704073843.1750458-3-ivo.g.dimitrov.75@gmail.com>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [High] Renaming the expected IRQ name from 'se1' to 'chrg_det' breaks compatibility with existing Device Tree binaries.
- [Medium] Early update of `ddata->mode` causes the state machine to become permanently stuck if the subsequent hardware configuration fails.
- [Medium] The system's SysRq functionality is permanently left disabled globally if a hardware configuration step fails in `cpcap_usb_set_idle_mode()`.
- [Medium] The module parameter `enable_uart` is ignored on boot, leaving the PHY in a partially uninitialized state.
- [Medium] A Time-of-Check to Time-of-Use (TOCTOU) race condition exists when reading the global `cpcap_enable_uart` module parameter.
Pre-existing issues:
- [Critical] A race condition during driver removal allows the devm-managed threaded IRQ handler to schedule `detect_work` after it has been canceled, leading to a Use-After-Free.
--
commit e8047562da705a40f07fe646c6694386f4dad156
Author: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
phy: cpcap-usb: add DCP detection and make UART idle mode optional
Handle DCP separately from USB host connections based on CPCAP charger
detection status.
Make the existing idle UART mode optional via the "enable_uart" module
parameter. When disabled, the PHY remains in its USB/charger detection
configuration while idle. This reduces power usage in idle by 25mW.
Also initialize the PHY into the baseline configuration required for
reliable charger detection during probe.
> diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
> index c6cc8fce6940f..3be2b3afaaf4d 100644
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c
> @@ -296,36 +309,53 @@ static void cpcap_usb_detect(struct work_struct *work)
> return;
> }
>
> vbus = cpcap_usb_vbus_valid(ddata);
>
> - /* Otherwise assume we're connected to a USB host */
> - if (vbus) {
> - dev_dbg(ddata->dev, "connected to USB host\n");
> - error = cpcap_usb_set_usb_mode(ddata);
> + /* Check if we're connected to a USB host */
> + if (vbus && ddata->mode != CPCAP_USB_HOST) {
> + if (!s.chrg_det || !s.sessvld || ddata->mode != CPCAP_IDLE)
> + return;
> +
> + if (s.se1)
> + ddata->mode = CPCAP_CHARGER;
> + else if (!s.dm && !s.dp)
> + ddata->mode = CPCAP_USB;
> + else
> + return;
[Severity: Medium]
Can the state machine become permanently stuck if the subsequent hardware
configuration fails?
The state variable ddata->mode is eagerly updated here to CPCAP_CHARGER or
CPCAP_USB before applying the hardware state via cpcap_usb_set_usb_mode().
If the configuration fails and returns early, the state is never reverted.
On the next evaluation, the condition checking ddata->mode != CPCAP_IDLE
will evaluate to true and return immediately, preventing recovery until the
cable is physically unplugged.
> +
> + dev_dbg(ddata->dev, "connected to %s\n",
> + s.se1 ? "DCP" : "USB host");
> +
> + error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC3,
> + CPCAP_BIT_VBUSSTBY_EN |
> + CPCAP_BIT_VBUSEN_SPI, 0);
> if (error)
> goto out_err;
> - cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_VALID);
> +
> + if (ddata->mode == CPCAP_CHARGER)
> + error = cpcap_usb_set_dcp_mode(ddata);
> + else
> + error = cpcap_usb_set_usb_mode(ddata);
> +
> + if (error)
> + goto out_err;
> +
> + if (ddata->mode == CPCAP_USB)
> + cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_VALID);
>
> return;
> }
>
> - ddata->vbus_provider = false;
> - ddata->docked = false;
> + if (ddata->mode == CPCAP_IDLE)
> + return;
[Severity: Medium]
Does this early return prevent the enable_uart module parameter from being
applied on boot?
During initial probe, ddata->mode is zero-initialized to CPCAP_IDLE. When
cpcap_usb_detect() runs for the first time, it exits early here and completely
skips cpcap_usb_set_idle_mode(). This leaves the PHY in a partially
uninitialized state until a cable is attached and removed.
> +
> + ddata->mode = CPCAP_IDLE;
> cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_OFF);
>
> - /* Default to debug UART mode */
> - error = cpcap_usb_set_uart_mode(ddata);
> + error = cpcap_usb_set_idle_mode(ddata);
> if (error)
> goto out_err;
[ ... ]
> @@ -374,7 +404,7 @@ static const char * const cpcap_phy_irqs[] = {
> "id_ground", "id_float",
>
> /* REG_INT1 */
> - "se0conn", "vbusvld", "sessvld", "sessend", "se1",
> + "se0conn", "vbusvld", "sessvld", "sessend", "chrg_det",
[Severity: High]
Does this break compatibility with existing Device Tree binaries?
The driver now strictly requests the "chrg_det" interrupt by name, but older
DTBs only provide "se1". Without a fallback to check for the old name,
cpcap_usb_init_irq() will return -ENODEV and abort the probe.
[ ... ]
> @@ -426,30 +457,52 @@ static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata)
> if (error)
> goto out_err;
>
> - if (ddata->pins_uart) {
> - error = pinctrl_select_state(ddata->pins, ddata->pins_uart);
> + if (cpcap_enable_uart && ddata->pins_uart) {
[Severity: Medium]
Can a race condition occur when reading the global cpcap_enable_uart module
parameter?
Since it is modifiable via sysfs and evaluated multiple times independently
within cpcap_usb_set_idle_mode(), a concurrent write could change its value
mid-execution. This might result in a mismatched configuration, such as
setting UART pinctrl but then improperly applying SUSPEND_SPI later in the
function.
> + error = pinctrl_select_state(ddata->pins, ddata->pins_uart);
> + if (error)
> + goto out_err;
> + } else if (!cpcap_enable_uart && ddata->pins_ulpi) {
> + error = pinctrl_select_state(ddata->pins, ddata->pins_ulpi);
> if (error)
> goto out_err;
> }
[ ... ]
> @@ -457,6 +510,8 @@ static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata)
> if (sysrq & 1)
> sysrq_toggle_support(sysrq);
>
> + dev_dbg(ddata->dev, "set %s mode\n", cpcap_enable_uart ? "UART" : "IDLE");
> +
> return 0;
>
> out_err:
> + dev_err(ddata->dev, "%s failed with %i\n", __func__, error);
> +
> + return error;
> +}
[Severity: Medium]
Is it possible for the system's SysRq functionality to be permanently left
disabled on an error path?
In cpcap_usb_set_idle_mode(), sysrq is disabled to prevent line glitches.
If any subsequent hardware configuration step fails, the function jumps to the
out_err label here and returns without restoring the original sysrq state via
sysrq_toggle_support().
[ ... ]
> @@ -697,13 +848,9 @@ static int cpcap_usb_phy_probe(struct platform_device *pdev)
> static void cpcap_usb_phy_remove(struct platform_device *pdev)
> {
> struct cpcap_phy_ddata *ddata = platform_get_drvdata(pdev);
> - int error;
>
> atomic_set(&ddata->active, 0);
> - error = cpcap_usb_set_uart_mode(ddata);
> - if (error)
> - dev_err(ddata->dev, "could not set UART mode\n");
> -
> + cpcap_usb_set_idle_mode(ddata);
> cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_OFF);
>
> usb_remove_phy(&ddata->phy);
[Severity: Critical]
This is a pre-existing issue, but does a race condition exist during driver
removal that allows the devm-managed threaded IRQ handler to schedule
detect_work after it has been canceled?
Because the IRQ is devm-managed, it is still active when
cancel_delayed_work_sync() is called at the end of cpcap_usb_phy_remove().
If the IRQ thread in cpcap_phy_irq_thread() evaluates ddata->active != 0
before it is cleared, but is preempted before calling schedule_delayed_work(),
the remove function can finish and cancel the work.
Devres will then wait for the preempted IRQ thread to finish. The thread will
resume and schedule the delayed work, and devres will then free ddata. When
the timer expires, the workqueue will execute cpcap_usb_detect() using the
freed ddata.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260704073843.1750458-1-ivo.g.dimitrov.75@gmail.com?part=2
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 1/5] phy: cpcap-usb: Prevent line glitches from triggering sysrq
From: sashiko-bot @ 2026-07-04 7:49 UTC (permalink / raw)
To: Ivaylo Dimitrov
Cc: vkoul, linux-phy, devicetree, conor+dt, neil.armstrong, olteanv,
robh
In-Reply-To: <20260704073843.1750458-2-ivo.g.dimitrov.75@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [High] The global SysRq support mask is permanently altered if an error occurs during UART mode configuration.
- [High] Flawed bitwise logic fails to effectively disable SysRq in system configurations that use specific feature masks, leaving the system vulnerable to glitches.
- [High] The driver modifies global system configuration to mask a local hardware glitch, introducing system-wide side effects and race conditions.
Pre-existing issues:
- [High] A Use-After-Free vulnerability exists because devres-managed IRQs can queue delayed work on freed memory after `remove()` completes.
- [High] Hardware teardown in `remove()` races with the background worker, leading to undefined hardware state.
--
commit d89e10225f70bc2771f7d996decb42920d32fe88
Author: Tony Lindgren <tony@atomide.com>
phy: cpcap-usb: Prevent line glitches from triggering sysrq
This commit attempts to prevent random sysrq events when switching from USB
to debug serial port mode. It temporarily disables sysrq during mode changes
to mask line glitches.
> diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c
[ ... ]
> @@ -413,7 +414,12 @@ static int cpcap_usb_gpio_set_mode(struct cpcap_phy_ddata *ddata,
>
> static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata)
> {
> - int error;
> + int sysrq, error;
> +
> + /* Disable sysrq to prevent random sysrq events on line glitches */
> + sysrq = sysrq_mask();
> + if (sysrq & 1)
> + sysrq_toggle_support(sysrq & ~1);
[Severity: High]
Does modifying the global sysrq state here create a race condition?
If an administrator concurrently modifies the sysrq settings via
/proc/sys/kernel/sysrq, will the restoration call at the end of this
function blindly overwrite their new state?
[Severity: High]
Will this correctly disable sysrq if the system is configured with a
specific feature mask?
If the administrator clears the SYSRQ_ENABLE_ALL bit (bit 0) but leaves
other features enabled, sysrq & 1 evaluates to 0, bypassing the disablement.
If bit 0 and other bits are set, sysrq & ~1 disables the 'enable all' flag
but leaves the specific logging/dumping features actively enabled.
[ ... ]
> @@ -448,6 +454,9 @@ static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata)
> if (error)
> goto out_err;
>
> + if (sysrq & 1)
> + sysrq_toggle_support(sysrq);
> +
> return 0;
>
> out_err:
[Severity: High]
Does this error path permanently alter the sysrq support mask?
If a failure occurs during the GPIO or pinctrl mode configuration, the
function jumps to out_err and bypasses the restoration of the sysrq state.
Could this permanently disable sysrq features system-wide?
[Severity: High]
This is a pre-existing issue, but is there a use-after-free vulnerability
during device teardown?
Looking at cpcap_phy_irq_thread():
drivers/phy/motorola/phy-cpcap-usb.c:cpcap_phy_irq_thread() {
if (!atomic_read(&ddata->active))
return IRQ_NONE;
schedule_delayed_work(&ddata->detect_work, msecs_to_jiffies(1));
}
And in cpcap_usb_phy_remove(), cancel_delayed_work_sync() is called, but
because devm_request_threaded_irq() is used, the IRQ is freed after
remove() finishes.
If the IRQ thread is preempted immediately after reading ddata->active,
can it resume during free_irq(), and queue detect_work on freed memory
after devres frees ddata?
[Severity: High]
This is a pre-existing issue, but does the hardware teardown in
cpcap_usb_phy_remove() race with the background worker?
Looking at cpcap_usb_phy_remove():
drivers/phy/motorola/phy-cpcap-usb.c:cpcap_usb_phy_remove() {
cpcap_usb_set_idle_mode(ddata);
cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_OFF);
usb_remove_phy(&ddata->phy);
cancel_delayed_work_sync(&ddata->detect_work);
}
The driver attempts to force an idle state before stopping the background
worker cpcap_usb_detect().
Since the worker thread doesn't check ddata->active, could it concurrently
modify the same regmap registers and pinctrl states, leaving the hardware
in an undefined state?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260704073843.1750458-1-ivo.g.dimitrov.75@gmail.com?part=1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 4/5] ARM: dts: ti: cpcap-mapphone: use charger detection interrupt for CPCAP USB PHY
From: sashiko-bot @ 2026-07-04 7:48 UTC (permalink / raw)
To: Ivaylo Dimitrov
Cc: vkoul, conor+dt, devicetree, robh, neil.armstrong, olteanv,
linux-phy
In-Reply-To: <20260704073843.1750458-5-ivo.g.dimitrov.75@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Backward Compatibility Breakage in DT interrupt name change
- [High] Missing Cleanup / Error Handling causes permanent sysrq disablement
- [Medium] Data Race on `cpcap_enable_uart` module parameter leads to mixed hardware state
--
commit 657e07c7931cc99eefeab1abc1d21d20f3271aa2
Author: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
ARM: dts: ti: cpcap-mapphone: use charger detection interrupt for CPCAP USB PHY
This commit updates the CPCAP USB PHY interrupt mapping in the device tree
to use the charger detection interrupt. It replaces the "se1" interrupt name
with "chrg_det", which matches driver and binding updates for DCP detection.
> diff --git a/arch/arm/boot/dts/ti/omap/motorola-cpcap-mapphone.dtsi b/arch/arm/boot/dts/ti/omap/motorola-cpcap-mapphone.dtsi
> index 73e27b95105e1..f3ec85d73a16f 100644
> --- a/arch/arm/boot/dts/ti/omap/motorola-cpcap-mapphone.dtsi
> +++ b/arch/arm/boot/dts/ti/omap/motorola-cpcap-mapphone.dtsi
[ ... ]
> @@ -106,11 +106,11 @@ cpcap_usb2_phy: phy {
> interrupts-extended =
> <&cpcap 15 0>, <&cpcap 14 0>, <&cpcap 28 0>,
> <&cpcap 19 0>, <&cpcap 18 0>, <&cpcap 17 0>,
> - <&cpcap 16 0>, <&cpcap 49 0>, <&cpcap 48 0>;
> + <&cpcap 13 0>, <&cpcap 49 0>, <&cpcap 48 0>;
> interrupt-names =
> "id_ground", "id_float", "se0conn",
> "vbusvld", "sessvld", "sessend",
> - "se1", "dm", "dp";
> + "chrg_det", "dm", "dp";
[Severity: High]
Does replacing the "se1" interrupt name outright instead of appending it
break backward compatibility?
If a new kernel boots with an older device tree (which provides "se1"
instead of "chrg_det"), probe aborts. The driver strictly requires all
interrupts to be present:
drivers/phy/motorola/phy-cpcap-usb.c:cpcap_phy_irqs:
/* REG_INT1 */
"se0conn", "vbusvld", "sessvld", "sessend", "chrg_det",
And in cpcap_usb_init_irq():
irq = platform_get_irq_byname(pdev, name);
if (irq < 0)
return -ENODEV;
Probe is aborted if "chrg_det" is not found, without attempting to fall
back to "se1".
[Severity: High]
Can a transient error during PHY mode switching in the driver permanently
disable sysrq?
In drivers/phy/motorola/phy-cpcap-usb.c:cpcap_usb_set_idle_mode(), sysrq is
disabled at the start:
sysrq = sysrq_mask();
if (sysrq & 1)
sysrq_toggle_support(sysrq & ~1);
error = cpcap_usb_gpio_set_mode(ddata, CPCAP_UNKNOWN_DISABLED);
if (error)
goto out_err;
If hardware configuration fails, the function skips the restoration block:
if (sysrq & 1)
sysrq_toggle_support(sysrq);
return 0;
out_err:
dev_err(ddata->dev, "%s failed with %i\n", __func__, error);
return error;
Does this silently strip the system of its diagnostic capabilities on a
single I2C/SPI bus error?
[Severity: Medium]
Could a concurrent modification to the cpcap_enable_uart module parameter
cause a mixed hardware state?
In drivers/phy/motorola/phy-cpcap-usb.c:cpcap_usb_set_idle_mode(), the global
cpcap_enable_uart is queried independently multiple times:
if (cpcap_enable_uart && ddata->pins_uart) {
error = pinctrl_select_state(ddata->pins, ddata->pins_uart);
...
if (cpcap_enable_uart)
val |= (CPCAP_BIT_UARTMUX0 | CPCAP_BIT_EMUMODE0);
...
if (!cpcap_enable_uart)
val |= CPCAP_BIT_SUSPEND_SPI;
If modified during execution, could this read true during the pinctrl check
and false during the regmap bitmask construction, leaving the hardware in an
inconsistent configuration?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260704073843.1750458-1-ivo.g.dimitrov.75@gmail.com?part=4
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 3/5] dt-bindings: phy: motorola,cpcap-usb: replace se1 interrupt with chrg_det
From: sashiko-bot @ 2026-07-04 7:47 UTC (permalink / raw)
To: Ivaylo Dimitrov
Cc: vkoul, conor+dt, neil.armstrong, linux-phy, olteanv, robh,
devicetree
In-Reply-To: <20260704073843.1750458-4-ivo.g.dimitrov.75@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Critical] Device Tree ABI breakage: replacing 'se1' interrupt with 'chrg_det' breaks probe on older DTBs.
- [Medium] Global SysRq state is permanently disabled on error paths.
Pre-existing issues:
- [High] Race condition in driver removal allows the threaded IRQ handler to schedule work on freed memory (Use-After-Free).
--
commit d7b2a51c3f82ebdb2f326313f13ddf2d3edde1a7
Author: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
dt-bindings: phy: motorola,cpcap-usb: replace se1 interrupt with chrg_det
The CPCAP USB PHY driver uses the CPCAP charger detection interrupt for DCP
detection rather than the SE1 interrupt. This commit updates the binding and
example DTS to use the corresponding "chrg_det" interrupt name.
> diff --git a/Documentation/devicetree/bindings/phy/motorola,cpcap-usb-phy.yaml b/Documentation/devicetree/bindings/phy/motorola,cpcap-usb-phy.yaml
> index dd345cbd0a0b4..d54cf7fbf11a7 100644
> --- a/Documentation/devicetree/bindings/phy/motorola,cpcap-usb-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/motorola,cpcap-usb-phy.yaml
> @@ -27,7 +27,7 @@ properties:
> - description: vbusvld interrupt
> - description: sessvld interrupt
> - description: sessend interrupt
> - - description: se1 interrupt
> + - description: charger detection interrupt
> - description: dm interrupt
> - description: dp interrupt
>
> @@ -40,7 +40,7 @@ properties:
> - const: vbusvld
> - const: sessvld
> - const: sessend
> - - const: se1
> + - const: chrg_det
[Severity: Critical]
Does replacing 'se1' with 'chrg_det' break probe for older device trees?
Since cpcap_usb_init_irq() in drivers/phy/motorola/phy-cpcap-usb.c fails the
probe if an expected interrupt is not found, updating the binding to require
'chrg_det' instead of 'se1' means booting a new kernel with an older DTB
(which only provides 'se1') could fail to probe the USB PHY.
> - const: dm
> - const: dp
>
[ ... ]
While reviewing this, I noticed a few other regressions in
drivers/phy/motorola/phy-cpcap-usb.c:
[Severity: Medium]
Can this error path in cpcap_usb_set_idle_mode() leave the global SysRq state
permanently disabled?
drivers/phy/motorola/phy-cpcap-usb.c:cpcap_usb_set_idle_mode() {
...
sysrq = sysrq_mask();
if (sysrq & 1)
sysrq_toggle_support(sysrq & ~1);
...
out_err:
dev_err(ddata->dev, "%s failed with %i\n", __func__, error);
return error;
}
If an error occurs (e.g., during pinctrl_select_state), the function jumps to
out_err and returns without calling sysrq_toggle_support(sysrq) to restore it.
[Severity: High]
This is a pre-existing issue, but could there be a race condition during
driver removal that allows the threaded IRQ handler to schedule work on freed
memory?
During driver removal, cpcap_usb_phy_remove() cancels the work without
disabling the IRQ first:
drivers/phy/motorola/phy-cpcap-usb.c:cpcap_usb_phy_remove() {
...
atomic_set(&ddata->active, 0);
...
cancel_delayed_work_sync(&ddata->detect_work);
regulator_disable(ddata->vusb);
}
Concurrently, if an IRQ fires and cpcap_phy_irq_thread() checks 'active' but
is preempted before scheduling the work:
drivers/phy/motorola/phy-cpcap-usb.c:cpcap_phy_irq_thread() {
...
if (!atomic_read(&ddata->active))
return IRQ_NONE;
schedule_delayed_work(&ddata->detect_work, msecs_to_jiffies(1));
...
}
Could the IRQ thread resume after cancel_delayed_work_sync() has completed,
scheduling the delayed work on a freed 'ddata' object?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260704073843.1750458-1-ivo.g.dimitrov.75@gmail.com?part=3
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH 5/5] phy: cpcap-usb: add extcon support
From: Ivaylo Dimitrov @ 2026-07-04 7:38 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Linus Walleij, Bartosz Golaszewski
Cc: linux-phy, devicetree, linux-kernel, linux-omap, linux-gpio,
Ivaylo Dimitrov
In-Reply-To: <20260704073843.1750458-1-ivo.g.dimitrov.75@gmail.com>
Register an Extcon device and report the detected cable state.
The driver already determines the type of cable attached during USB cable
detection. Export the detected state through the Extcon framework so
other drivers can consume it using a standard kernel interface.
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
drivers/phy/motorola/phy-cpcap-usb.c | 68 ++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
index 3be2b3afaaf4..8978a95c1c19 100644
--- a/drivers/phy/motorola/phy-cpcap-usb.c
+++ b/drivers/phy/motorola/phy-cpcap-usb.c
@@ -12,6 +12,7 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/err.h>
+#include <linux/extcon-provider.h>
#include <linux/io.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -123,6 +124,7 @@ struct cpcap_phy_ddata {
struct regmap *reg;
struct device *dev;
struct usb_phy phy;
+ struct extcon_dev *edev;
struct delayed_work detect_work;
struct pinctrl *pins;
struct pinctrl_state *pins_ulpi;
@@ -134,12 +136,23 @@ struct cpcap_phy_ddata {
struct regulator *vusb;
atomic_t active;
enum cpcap_mode mode;
+ int cable;
};
static bool cpcap_enable_uart;
module_param_named(enable_uart, cpcap_enable_uart, bool, 0644);
MODULE_PARM_DESC(enable_uart,
"Enable UART on the USB connector while idle (increases power consumption)");
+
+/* List of detectable cables */
+static const unsigned int cpcap_extcon_cables[] = {
+ EXTCON_USB,
+ EXTCON_USB_HOST,
+ EXTCON_CHG_USB_SDP,
+ EXTCON_CHG_USB_DCP,
+ EXTCON_NONE,
+};
+
static bool cpcap_usb_vbus_valid(struct cpcap_phy_ddata *ddata)
{
int error, value = 0;
@@ -226,6 +239,43 @@ static void cpcap_usb_try_musb_mailbox(struct cpcap_phy_ddata *ddata,
__func__, error);
}
+static void cpcap_usb_set_extcon(struct cpcap_phy_ddata *ddata)
+{
+ int cable;
+
+ switch (ddata->mode) {
+ case CPCAP_CHARGER:
+ cable = EXTCON_CHG_USB_DCP;
+ break;
+ case CPCAP_USB:
+ cable = EXTCON_CHG_USB_SDP;
+ break;
+ case CPCAP_USB_HOST:
+ case CPCAP_DOCK:
+ cable = EXTCON_USB_HOST;
+ break;
+ case CPCAP_IDLE:
+ cable = EXTCON_NONE;
+ break;
+ }
+
+ if (cable == ddata->cable)
+ return;
+
+ if (ddata->cable == EXTCON_CHG_USB_SDP)
+ extcon_set_state(ddata->edev, EXTCON_USB, false);
+ else if (cable == EXTCON_CHG_USB_SDP)
+ extcon_set_state(ddata->edev, EXTCON_USB, true);
+
+ if (ddata->cable != EXTCON_NONE)
+ extcon_set_state_sync(ddata->edev, ddata->cable, false);
+
+ if (cable != EXTCON_NONE)
+ extcon_set_state_sync(ddata->edev, cable, true);
+
+ ddata->cable = cable;
+}
+
static void cpcap_usb_detect(struct work_struct *work)
{
struct cpcap_phy_ddata *ddata;
@@ -276,6 +326,7 @@ static void cpcap_usb_detect(struct work_struct *work)
goto out_err;
cpcap_usb_try_musb_mailbox(ddata, MUSB_ID_GROUND);
+ cpcap_usb_set_extcon(ddata);
/*
* Force check state again after musb has reoriented,
@@ -306,6 +357,8 @@ static void cpcap_usb_detect(struct work_struct *work)
if (error)
goto out_err;
+ cpcap_usb_set_extcon(ddata);
+
return;
}
@@ -343,6 +396,8 @@ static void cpcap_usb_detect(struct work_struct *work)
if (ddata->mode == CPCAP_USB)
cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_VALID);
+ cpcap_usb_set_extcon(ddata);
+
return;
}
@@ -356,6 +411,8 @@ static void cpcap_usb_detect(struct work_struct *work)
if (error)
goto out_err;
+ cpcap_usb_set_extcon(ddata);
+
return;
out_err:
@@ -781,11 +838,22 @@ static int cpcap_usb_phy_probe(struct platform_device *pdev)
if (!otg)
return -ENOMEM;
+ ddata->edev = devm_extcon_dev_allocate(&pdev->dev, cpcap_extcon_cables);
+ if (IS_ERR(ddata->edev)) {
+ return dev_err_probe(&pdev->dev, PTR_ERR(ddata->edev),
+ "failed to allocate extcon device\n");
+ }
+
+ error = devm_extcon_dev_register(&pdev->dev, ddata->edev);
+ if (error < 0)
+ return error;
+
ddata->dev = &pdev->dev;
ddata->phy.dev = ddata->dev;
ddata->phy.label = "cpcap_usb_phy";
ddata->phy.otg = otg;
ddata->phy.type = USB_PHY_TYPE_USB2;
+ ddata->cable = EXTCON_NONE;
otg->set_host = cpcap_usb_phy_set_host;
otg->set_peripheral = cpcap_usb_phy_set_peripheral;
otg->usb_phy = &ddata->phy;
--
2.25.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH 4/5] ARM: dts: ti: cpcap-mapphone: use charger detection interrupt for CPCAP USB PHY
From: Ivaylo Dimitrov @ 2026-07-04 7:38 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Linus Walleij, Bartosz Golaszewski
Cc: linux-phy, devicetree, linux-kernel, linux-omap, linux-gpio,
Ivaylo Dimitrov
In-Reply-To: <20260704073843.1750458-1-ivo.g.dimitrov.75@gmail.com>
Update the CPCAP USB PHY interrupt mapping to use the charger
detection interrupt and corresponding "chrg_det" interrupt name.
This matches the driver and binding updates for DCP detection.
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
arch/arm/boot/dts/ti/omap/motorola-cpcap-mapphone.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/ti/omap/motorola-cpcap-mapphone.dtsi b/arch/arm/boot/dts/ti/omap/motorola-cpcap-mapphone.dtsi
index 6b1195c2a768..b75a327caf1f 100644
--- a/arch/arm/boot/dts/ti/omap/motorola-cpcap-mapphone.dtsi
+++ b/arch/arm/boot/dts/ti/omap/motorola-cpcap-mapphone.dtsi
@@ -125,11 +125,11 @@ cpcap_usb2_phy: phy {
interrupts-extended =
<&cpcap 15 0>, <&cpcap 14 0>, <&cpcap 28 0>,
<&cpcap 19 0>, <&cpcap 18 0>, <&cpcap 17 0>,
- <&cpcap 16 0>, <&cpcap 49 0>, <&cpcap 48 0>;
+ <&cpcap 13 0>, <&cpcap 49 0>, <&cpcap 48 0>;
interrupt-names =
"id_ground", "id_float", "se0conn",
"vbusvld", "sessvld", "sessend",
- "se1", "dm", "dp";
+ "chrg_det", "dm", "dp";
mode-gpios = <&gpio2 28 GPIO_ACTIVE_HIGH>,
<&gpio1 0 GPIO_ACTIVE_HIGH>;
io-channels = <&cpcap_adc 2>, <&cpcap_adc 7>;
--
2.25.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH 3/5] dt-bindings: phy: motorola,cpcap-usb: replace se1 interrupt with chrg_det
From: Ivaylo Dimitrov @ 2026-07-04 7:38 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Linus Walleij, Bartosz Golaszewski
Cc: linux-phy, devicetree, linux-kernel, linux-omap, linux-gpio,
Ivaylo Dimitrov
In-Reply-To: <20260704073843.1750458-1-ivo.g.dimitrov.75@gmail.com>
The CPCAP USB PHY driver uses the CPCAP charger detection interrupt
for DCP detection rather than the SE1 interrupt.
Update the binding and example DTS to use the corresponding
"chrg_det" interrupt name.arch: arm: dts: cpcap-mapphone: xxx
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
.../devicetree/bindings/phy/motorola,cpcap-usb-phy.yaml | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/phy/motorola,cpcap-usb-phy.yaml b/Documentation/devicetree/bindings/phy/motorola,cpcap-usb-phy.yaml
index 0febd04a61f4..81c5f3b5997b 100644
--- a/Documentation/devicetree/bindings/phy/motorola,cpcap-usb-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/motorola,cpcap-usb-phy.yaml
@@ -27,7 +27,7 @@ properties:
- description: vbusvld interrupt
- description: sessvld interrupt
- description: sessend interrupt
- - description: se1 interrupt
+ - description: charger detection interrupt
- description: dm interrupt
- description: dp interrupt
@@ -40,7 +40,7 @@ properties:
- const: vbusvld
- const: sessvld
- const: sessend
- - const: se1
+ - const: chrg_det
- const: dm
- const: dp
@@ -90,11 +90,11 @@ examples:
#phy-cells = <0>;
interrupts-extended = <
&cpcap 15 0 &cpcap 14 0 &cpcap 28 0 &cpcap 19 0
- &cpcap 18 0 &cpcap 17 0 &cpcap 16 0 &cpcap 49 0
+ &cpcap 18 0 &cpcap 17 0 &cpcap 13 0 &cpcap 49 0
&cpcap 48 1
>;
interrupt-names = "id_ground", "id_float", "se0conn", "vbusvld",
- "sessvld", "sessend", "se1", "dm", "dp";
+ "sessvld", "sessend", "chrg_det", "dm", "dp";
io-channels = <&cpcap_adc 2>, <&cpcap_adc 7>;
io-channel-names = "vbus", "id";
vusb-supply = <&vusb>;
--
2.25.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH 2/5] phy: cpcap-usb: add DCP detection and make UART idle mode optional
From: Ivaylo Dimitrov @ 2026-07-04 7:38 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Linus Walleij, Bartosz Golaszewski
Cc: linux-phy, devicetree, linux-kernel, linux-omap, linux-gpio,
Ivaylo Dimitrov
In-Reply-To: <20260704073843.1750458-1-ivo.g.dimitrov.75@gmail.com>
Handle DCP separately from USB host connections based on CPCAP charger
detection status.
Make the existing idle UART mode optional via the "enable_uart" module
parameter. When disabled, the PHY remains in its USB/charger detection
configuration while idle. This reduces power usage in idle by 25mW.
Also initialize the PHY into the baseline configuration required for
reliable charger detection during probe.
Note: Enabling UART idle mode increases idle power consumption.
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
---
drivers/phy/motorola/phy-cpcap-usb.c | 235 ++++++++++++++++++++++-----
1 file changed, 191 insertions(+), 44 deletions(-)
diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
index c6cc8fce6940..3be2b3afaaf4 100644
--- a/drivers/phy/motorola/phy-cpcap-usb.c
+++ b/drivers/phy/motorola/phy-cpcap-usb.c
@@ -111,6 +111,14 @@ enum cpcap_gpio_mode {
CPCAP_OTG_DM_DP,
};
+enum cpcap_mode {
+ CPCAP_IDLE,
+ CPCAP_CHARGER,
+ CPCAP_USB,
+ CPCAP_USB_HOST,
+ CPCAP_DOCK,
+};
+
struct cpcap_phy_ddata {
struct regmap *reg;
struct device *dev;
@@ -125,10 +133,13 @@ struct cpcap_phy_ddata {
struct iio_channel *id;
struct regulator *vusb;
atomic_t active;
- unsigned int vbus_provider:1;
- unsigned int docked:1;
+ enum cpcap_mode mode;
};
+static bool cpcap_enable_uart;
+module_param_named(enable_uart, cpcap_enable_uart, bool, 0644);
+MODULE_PARM_DESC(enable_uart,
+ "Enable UART on the USB connector while idle (increases power consumption)");
static bool cpcap_usb_vbus_valid(struct cpcap_phy_ddata *ddata)
{
int error, value = 0;
@@ -176,6 +187,7 @@ static int cpcap_phy_get_ints_state(struct cpcap_phy_ddata *ddata,
s->id_ground = val & BIT(15);
s->id_float = val & BIT(14);
+ s->chrg_det = val & BIT(13);
s->vbusov = val & BIT(11);
error = regmap_read(ddata->reg, CPCAP_REG_INTS2, &val);
@@ -197,8 +209,9 @@ static int cpcap_phy_get_ints_state(struct cpcap_phy_ddata *ddata,
return 0;
}
-static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata);
+static int cpcap_usb_set_idle_mode(struct cpcap_phy_ddata *ddata);
static int cpcap_usb_set_usb_mode(struct cpcap_phy_ddata *ddata);
+static int cpcap_usb_set_dcp_mode(struct cpcap_phy_ddata *ddata);
static void cpcap_usb_try_musb_mailbox(struct cpcap_phy_ddata *ddata,
enum musb_vbus_id_status status)
@@ -229,7 +242,7 @@ static void cpcap_usb_detect(struct work_struct *work)
vbus = cpcap_usb_vbus_valid(ddata);
/* We need to kick the VBUS as USB A-host */
- if (s.id_ground && ddata->vbus_provider) {
+ if (s.id_ground && ddata->mode == CPCAP_USB_HOST) {
dev_dbg(ddata->dev, "still in USB A-host mode, kicking VBUS\n");
cpcap_usb_try_musb_mailbox(ddata, MUSB_ID_GROUND);
@@ -244,7 +257,7 @@ static void cpcap_usb_detect(struct work_struct *work)
return;
}
- if (vbus && s.id_ground && ddata->docked) {
+ if (vbus && s.id_ground && ddata->mode == CPCAP_DOCK) {
dev_dbg(ddata->dev, "still docked as A-host, signal ID down\n");
cpcap_usb_try_musb_mailbox(ddata, MUSB_ID_GROUND);
@@ -253,10 +266,10 @@ static void cpcap_usb_detect(struct work_struct *work)
}
/* No VBUS needed with docks */
- if (vbus && s.id_ground && !ddata->vbus_provider) {
+ if (vbus && s.id_ground && ddata->mode != CPCAP_USB_HOST) {
dev_dbg(ddata->dev, "connected to a dock\n");
- ddata->docked = true;
+ ddata->mode = CPCAP_DOCK;
error = cpcap_usb_set_usb_mode(ddata);
if (error)
@@ -275,10 +288,10 @@ static void cpcap_usb_detect(struct work_struct *work)
return;
}
- if (s.id_ground && !ddata->docked) {
+ if (s.id_ground && ddata->mode != CPCAP_DOCK) {
dev_dbg(ddata->dev, "id ground, USB host mode\n");
- ddata->vbus_provider = true;
+ ddata->mode = CPCAP_USB_HOST;
error = cpcap_usb_set_usb_mode(ddata);
if (error)
@@ -296,36 +309,53 @@ static void cpcap_usb_detect(struct work_struct *work)
return;
}
- error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC3,
- CPCAP_BIT_VBUSSTBY_EN |
- CPCAP_BIT_VBUSEN_SPI, 0);
- if (error)
- goto out_err;
-
vbus = cpcap_usb_vbus_valid(ddata);
- /* Otherwise assume we're connected to a USB host */
- if (vbus) {
- dev_dbg(ddata->dev, "connected to USB host\n");
- error = cpcap_usb_set_usb_mode(ddata);
+ /* Check if we're connected to a USB host */
+ if (vbus && ddata->mode != CPCAP_USB_HOST) {
+ if (!s.chrg_det || !s.sessvld || ddata->mode != CPCAP_IDLE)
+ return;
+
+ if (s.se1)
+ ddata->mode = CPCAP_CHARGER;
+ else if (!s.dm && !s.dp)
+ ddata->mode = CPCAP_USB;
+ else
+ return;
+
+ dev_dbg(ddata->dev, "connected to %s\n",
+ s.se1 ? "DCP" : "USB host");
+
+ error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC3,
+ CPCAP_BIT_VBUSSTBY_EN |
+ CPCAP_BIT_VBUSEN_SPI, 0);
if (error)
goto out_err;
- cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_VALID);
+
+ if (ddata->mode == CPCAP_CHARGER)
+ error = cpcap_usb_set_dcp_mode(ddata);
+ else
+ error = cpcap_usb_set_usb_mode(ddata);
+
+ if (error)
+ goto out_err;
+
+ if (ddata->mode == CPCAP_USB)
+ cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_VALID);
return;
}
- ddata->vbus_provider = false;
- ddata->docked = false;
+ if (ddata->mode == CPCAP_IDLE)
+ return;
+
+ ddata->mode = CPCAP_IDLE;
cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_OFF);
- /* Default to debug UART mode */
- error = cpcap_usb_set_uart_mode(ddata);
+ error = cpcap_usb_set_idle_mode(ddata);
if (error)
goto out_err;
- dev_dbg(ddata->dev, "set UART mode\n");
-
return;
out_err:
@@ -374,7 +404,7 @@ static const char * const cpcap_phy_irqs[] = {
"id_ground", "id_float",
/* REG_INT1 */
- "se0conn", "vbusvld", "sessvld", "sessend", "se1",
+ "se0conn", "vbusvld", "sessvld", "sessend", "chrg_det",
/* REG_INT_3 */
"dm", "dp",
@@ -412,9 +442,10 @@ static int cpcap_usb_gpio_set_mode(struct cpcap_phy_ddata *ddata,
return 0;
}
-static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata)
+static int cpcap_usb_set_idle_mode(struct cpcap_phy_ddata *ddata)
{
int sysrq, error;
+ unsigned int val;
/* Disable sysrq to prevent random sysrq events on line glitches */
sysrq = sysrq_mask();
@@ -426,30 +457,52 @@ static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata)
if (error)
goto out_err;
- if (ddata->pins_uart) {
- error = pinctrl_select_state(ddata->pins, ddata->pins_uart);
+ if (cpcap_enable_uart && ddata->pins_uart) {
+ error = pinctrl_select_state(ddata->pins, ddata->pins_uart);
+ if (error)
+ goto out_err;
+ } else if (!cpcap_enable_uart && ddata->pins_ulpi) {
+ error = pinctrl_select_state(ddata->pins, ddata->pins_ulpi);
if (error)
goto out_err;
}
error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC1,
+ CPCAP_BIT_DP150KPU |
+ CPCAP_BIT_DP1K5PU |
+ CPCAP_BIT_DM1K5PU |
+ CPCAP_BIT_DPPD |
+ CPCAP_BIT_DMPD |
CPCAP_BIT_VBUSPD,
+ CPCAP_BIT_DP150KPU |
CPCAP_BIT_VBUSPD);
if (error)
goto out_err;
- error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC2,
- 0xffff, CPCAP_BIT_UARTMUX0 |
- CPCAP_BIT_EMUMODE0);
+ val = CPCAP_BIT_USBSUSPEND;
+
+ if (cpcap_enable_uart)
+ val |= (CPCAP_BIT_UARTMUX0 | CPCAP_BIT_EMUMODE0);
+
+ error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC2, 0xffff, val);
if (error)
goto out_err;
- error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC3, 0x7fff,
- CPCAP_BIT_IDPU_SPI);
+ val = CPCAP_BIT_VBUSSTBY_EN |
+ CPCAP_BIT_VBUSPD_SPI |
+ CPCAP_BIT_DMPD_SPI |
+ CPCAP_BIT_DPPD_SPI |
+ CPCAP_BIT_PU_SPI |
+ CPCAP_BIT_IDPU_SPI |
+ CPCAP_BIT_ULPI_SPI_SEL;
+
+ if (!cpcap_enable_uart)
+ val |= CPCAP_BIT_SUSPEND_SPI;
+
+ error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC3, 0x7fff, val);
if (error)
goto out_err;
- /* Enable UART mode */
error = cpcap_usb_gpio_set_mode(ddata, CPCAP_DM_DP);
if (error)
goto out_err;
@@ -457,6 +510,8 @@ static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata)
if (sysrq & 1)
sysrq_toggle_support(sysrq);
+ dev_dbg(ddata->dev, "set %s mode\n", cpcap_enable_uart ? "UART" : "IDLE");
+
return 0;
out_err:
@@ -517,6 +572,42 @@ static int cpcap_usb_set_usb_mode(struct cpcap_phy_ddata *ddata)
return error;
}
+static int cpcap_usb_set_dcp_mode(struct cpcap_phy_ddata *ddata)
+{
+ int error = 0;
+
+ /* Disable lines to prevent glitches from waking up mdm6600 */
+ error = cpcap_usb_gpio_set_mode(ddata, CPCAP_UNKNOWN_DISABLED);
+ if (error)
+ return error;
+
+ if (ddata->pins_ulpi) {
+ error = pinctrl_select_state(ddata->pins, ddata->pins_ulpi);
+ if (error) {
+ dev_err(ddata->dev, "could not set usb mode: %i\n",
+ error);
+
+ return error;
+ }
+ }
+
+ error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC3,
+ CPCAP_BIT_SUSPEND_SPI, 0);
+ if (error)
+ goto out_err;
+
+ error = cpcap_usb_gpio_set_mode(ddata, CPCAP_DM_DP);
+ if (error)
+ goto out_err;
+
+ return 0;
+
+out_err:
+ dev_err(ddata->dev, "%s failed with %i\n", __func__, error);
+
+ return error;
+}
+
static int cpcap_usb_init_optional_pins(struct cpcap_phy_ddata *ddata)
{
ddata->pins = devm_pinctrl_get(ddata->dev);
@@ -546,9 +637,6 @@ static int cpcap_usb_init_optional_pins(struct cpcap_phy_ddata *ddata)
ddata->pins_uart = NULL;
}
- if (ddata->pins_uart)
- return pinctrl_select_state(ddata->pins, ddata->pins_uart);
-
return 0;
}
@@ -601,6 +689,65 @@ static int cpcap_usb_init_iio(struct cpcap_phy_ddata *ddata)
return error;
}
+static int cpcap_usb_init_usb(struct cpcap_phy_ddata *ddata)
+{
+ struct cpcap_usb_ints_state s;
+ int error;
+ int mask;
+
+ error = cpcap_phy_get_ints_state(ddata, &s);
+ if (error)
+ return error;
+
+ error = cpcap_usb_gpio_set_mode(ddata, CPCAP_UNKNOWN_DISABLED);
+ if (error)
+ return error;
+
+ if (ddata->pins_ulpi) {
+ error = pinctrl_select_state(ddata->pins, ddata->pins_ulpi);
+ if (error)
+ return error;
+ }
+
+ /*
+ * Do not touch DM/DP PD as this will break charger detection if charger
+ * is connected before we detect it.
+ */
+ if (s.se1) {
+ mask = CPCAP_BIT_VBUSEN_SPI | CPCAP_BIT_VBUSPU_SPI |
+ CPCAP_BIT_SUSPEND_SPI | CPCAP_BIT_ULPI_SPI_SEL;
+ } else {
+ mask = CPCAP_BIT_VBUSEN_SPI | CPCAP_BIT_VBUSPU_SPI |
+ CPCAP_BIT_DMPD_SPI | CPCAP_BIT_DPPD_SPI |
+ CPCAP_BIT_SUSPEND_SPI | CPCAP_BIT_PU_SPI |
+ CPCAP_BIT_ULPI_SPI_SEL;
+ }
+
+ error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC3, mask,
+ CPCAP_BIT_ULPI_SPI_SEL);
+ if (error)
+ return error;
+
+ /*
+ * Configure safe DP/DM termination state.
+ *
+ * Vendor kernel uses:
+ * DP150KPU = enabled
+ * all USB pullups/pulldowns = disabled
+ */
+ error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC1,
+ CPCAP_BIT_DP150KPU |
+ CPCAP_BIT_DP1K5PU |
+ CPCAP_BIT_DM1K5PU |
+ CPCAP_BIT_DPPD |
+ CPCAP_BIT_DMPD,
+ CPCAP_BIT_DP150KPU);
+ if (error)
+ return error;
+
+ return cpcap_usb_gpio_set_mode(ddata, CPCAP_DM_DP);
+}
+
#ifdef CONFIG_OF
static const struct of_device_id cpcap_usb_phy_id_table[] = {
{
@@ -678,6 +825,10 @@ static int cpcap_usb_phy_probe(struct platform_device *pdev)
if (error)
goto out_reg_disable;
+ error = cpcap_usb_init_usb(ddata);
+ if (error)
+ goto out_reg_disable;
+
error = cpcap_usb_init_interrupts(pdev, ddata);
if (error)
goto out_reg_disable;
@@ -697,13 +848,9 @@ static int cpcap_usb_phy_probe(struct platform_device *pdev)
static void cpcap_usb_phy_remove(struct platform_device *pdev)
{
struct cpcap_phy_ddata *ddata = platform_get_drvdata(pdev);
- int error;
atomic_set(&ddata->active, 0);
- error = cpcap_usb_set_uart_mode(ddata);
- if (error)
- dev_err(ddata->dev, "could not set UART mode\n");
-
+ cpcap_usb_set_idle_mode(ddata);
cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_OFF);
usb_remove_phy(&ddata->phy);
--
2.25.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH 0/5] phy: cpcap-usb: improve charger detection and export cable state
From: Ivaylo Dimitrov @ 2026-07-04 7:38 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Linus Walleij, Bartosz Golaszewski
Cc: linux-phy, devicetree, linux-kernel, linux-omap, linux-gpio,
Ivaylo Dimitrov
The Motorola CPCAP USB PHY contains the hardware state machine used for
USB cable detection. Besides distinguishing USB peripheral and host
connections, it can also detect dedicated charging ports (DCP).
This series starts with a prerequisite fix from Tony Lindgren to prevent
spurious SysRq events when switching between USB and UART modes. It then
adds DCP detection support to the CPCAP USB PHY, updates the Device Tree
binding and corresponding mapphone Device Tree to use the charger
detection interrupt, and finally exports the detected cable state through
the Extcon framework.
The existing driver already interprets the CPCAP USB detection state
machine to determine the attached cable type. This series extends that
logic to distinguish DCP connections and exposes the detected cable state
through Extcon using a standard kernel interface. It also makes the idle
UART mode optional, allowing the PHY to remain in its default USB
detection configuration unless UART support is explicitly requested.
The series has been tested on Motorola Droid 4 hardware.
Ivaylo Dimitrov (4):
phy: cpcap-usb: add DCP detection and make UART idle mode optional
dt-bindings: phy: motorola,cpcap-usb: replace se1 interrupt with
chrg_det
ARM: dts: ti: cpcap-mapphone: use charger detection interrupt for
CPCAP USB PHY
phy: cpcap-usb: add extcon support
Tony Lindgren (1):
phy: cpcap-usb: Prevent line glitches from triggering sysrq
.../bindings/phy/motorola,cpcap-usb-phy.yaml | 8 +-
.../dts/ti/omap/motorola-cpcap-mapphone.dtsi | 4 +-
drivers/phy/motorola/phy-cpcap-usb.c | 312 +++++++++++++++---
3 files changed, 274 insertions(+), 50 deletions(-)
--
2.25.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH 1/5] phy: cpcap-usb: Prevent line glitches from triggering sysrq
From: Ivaylo Dimitrov @ 2026-07-04 7:38 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Linus Walleij, Bartosz Golaszewski
Cc: linux-phy, devicetree, linux-kernel, linux-omap, linux-gpio
In-Reply-To: <20260704073843.1750458-1-ivo.g.dimitrov.75@gmail.com>
From: Tony Lindgren <tony@atomide.com>
We can get random sysrq events on switching from USB to debug serial
port mode unless is temporarily disable sysrq for the mode changes.
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
drivers/phy/motorola/phy-cpcap-usb.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
index 7cb020dd3423..c6cc8fce6940 100644
--- a/drivers/phy/motorola/phy-cpcap-usb.c
+++ b/drivers/phy/motorola/phy-cpcap-usb.c
@@ -20,6 +20,7 @@
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/slab.h>
+#include <linux/sysrq.h>
#include <linux/gpio/consumer.h>
#include <linux/mfd/motorola-cpcap.h>
@@ -413,7 +414,12 @@ static int cpcap_usb_gpio_set_mode(struct cpcap_phy_ddata *ddata,
static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata)
{
- int error;
+ int sysrq, error;
+
+ /* Disable sysrq to prevent random sysrq events on line glitches */
+ sysrq = sysrq_mask();
+ if (sysrq & 1)
+ sysrq_toggle_support(sysrq & ~1);
/* Disable lines to prevent glitches from waking up mdm6600 */
error = cpcap_usb_gpio_set_mode(ddata, CPCAP_UNKNOWN_DISABLED);
@@ -448,6 +454,9 @@ static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata)
if (error)
goto out_err;
+ if (sysrq & 1)
+ sysrq_toggle_support(sysrq);
+
return 0;
out_err:
--
2.25.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* Re: [PATCH v10 32/36] phy: rockchip: usbdp: Drop -EPROBE_DEFER hack
From: sashiko-bot @ 2026-07-04 0:12 UTC (permalink / raw)
To: Sebastian Reichel
Cc: devicetree, robh, olteanv, vkoul, neil.armstrong, linux-phy,
conor+dt
In-Reply-To: <20260703-rockchip-usbdp-cleanup-v10-32-a392711ca8a9@collabora.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The commit message incorrectly claims that the driver performs a reset during its probe routine, which can lead to a permanent DWC3 probe failure when initialized by U-Boot.
--
commit 6fe2e9f6ad72e10da5478abbe129b4525d89b28f
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
phy: rockchip: usbdp: Drop -EPROBE_DEFER hack
This commit removes the -EPROBE_DEFER fallback from rk_udphy_status_check().
It relies on a hardware reset in the probe routine to avoid the PLL locking
timeout.
> The hack to return -EPROBE_DEFER when the lcpll lock timeouts is no
> longer needed. The driver now does a reset during its probe routine
> marking everything as off and later on does a re-init, which avoids
> the problem.
[Severity: High]
Is this description accurate? Looking at rk_udphy_probe(), the hardware reset
appears to be missing.
If the bootloader (U-Boot) initializes and leaves the USB PHY active, and
rk_udphy_probe() fails to assert the hardware resets, the PHY remains active.
When DWC3 later probes and calls phy_init(), it attempts to re-initialize the
PHY.
Because DWC3 registers its PHY notifier after calling phy_init(), it misses
the PHY_NOTIFY_PRE_RESET event and fails to soft-reset the USB MAC. The
active DWC3 MAC can interfere with the PHY's PLL lock, causing the
rk_udphy_status_check() to timeout.
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 5d151fff9eeba..3fb6eea2979b9 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -750,17 +750,7 @@ static int rk_udphy_status_check(struct rk_udphy *udphy)
> (val & CMN_ANA_LCPLL_LOCK_DONE), 200, 100000);
> if (ret) {
> dev_err(udphy->dev, "cmn ana lcpll lock timeout\n");
> - /*
> - * If earlier software (U-Boot) enabled USB once already
> - * the PLL may have problems locking on the first try.
> - * It will be successful on the second try, so for the
> - * time being a -EPROBE_DEFER will solve the issue.
> - *
> - * This requires further investigation to understand the
> - * root cause, especially considering that the driver is
> - * asserting all reset lines at probe time.
> - */
> - return -EPROBE_DEFER;
> + return ret;
> }
[Severity: High]
Does propagating this timeout instead of returning -EPROBE_DEFER cause the
DWC3 probe to fail permanently rather than retrying?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-rockchip-usbdp-cleanup-v10-0-a392711ca8a9@collabora.com?part=32
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v10 36/36] phy: rockchip: usbdp: Add USB-C state without DP enabled
From: sashiko-bot @ 2026-07-04 0:01 UTC (permalink / raw)
To: Sebastian Reichel
Cc: devicetree, robh, vkoul, neil.armstrong, olteanv, linux-phy,
conor+dt
In-Reply-To: <20260703-rockchip-usbdp-cleanup-v10-36-a392711ca8a9@collabora.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Early return in `rk_udphy_mode_set` prevents `lane_mux_sel` from being updated when only the cable orientation changes.
- [Medium] Mapping `TYPEC_STATE_USB` to `UDPHY_MODE_USB` causes PHY resets and USB disconnections during DP Alt Mode entry.
Pre-existing issues:
- [High] Devres registration ordering causes a Use-After-Free of `phy_u3` during unbind.
--
commit 054870f51259caaa299758adc71548fa9bf71159
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
phy: rockchip: usbdp: Add USB-C state without DP enabled
This commit adds a separate state for USB-only mode without DisplayPort enabled
to improve power management. It splits the previous DP + USB3 combined mode
when DP AltMode is not present.
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -617,12 +599,40 @@ static void rk_udphy_set_typec_state(struct rk_udphy *udphy, unsigned long state
> udphy->lane_mux_sel[2] = PHY_LANE_MUX_DP;
> udphy->lane_mux_sel[3] = PHY_LANE_MUX_DP;
> }
> - hw_mode = UDPHY_MODE_DP_USB;
> - udphy->dp_lanes = 2;
> + }
> +}
> +
> +static void rk_udphy_mode_set(struct rk_udphy *udphy, u8 hw_mode, u8 dp_lanes)
> +{
> + if (udphy->hw_mode == hw_mode && udphy->dp_lanes == dp_lanes)
> + return;
[Severity: High]
Does this early return prevent updating the lane mux if only the cable
orientation changes?
If a cable is physically flipped, the code updates udphy->flip and calls
rk_udphy_set_typec_default_mapping(), which then sets the state to
TYPEC_DP_STATE_D.
If the PHY was already in UDPHY_MODE_DP_USB with 2 DP lanes, this check
causes an early return, skipping the newly moved rk_udphy_set_lane_mux()
call below. This would leave stale inverted lane assignments when
rk_udphy_init() subsequently configures the hardware.
> +
> + udphy->phy_needs_reinit = true;
> + udphy->hw_mode = hw_mode;
> + udphy->dp_lanes = dp_lanes;
> +
> + rk_udphy_set_lane_mux(udphy);
> +}
> +
> +static void rk_udphy_set_typec_state(struct rk_udphy *udphy, unsigned long state)
> +{
> + switch (state) {
> [ ... ]
> + case TYPEC_STATE_SAFE:
> + case TYPEC_STATE_USB:
> + default:
> + rk_udphy_mode_set(udphy, UDPHY_MODE_USB, 0);
> + break;
[Severity: Medium]
Will mapping this intermediate state to USB-only force a disruptive hardware
mode change when transitioning to DP Alt Mode?
Type-C port managers typically request TYPEC_STATE_USB upon connection.
Setting this to UDPHY_MODE_USB configures 0 DP lanes.
When DP Alt Mode successfully negotiates shortly after and transitions to
TYPEC_DP_STATE_D, hw_mode changes to UDPHY_MODE_DP_USB. This triggers a
full PHY reset in rk_udphy_update_power_state() and interrupts active
USB transfers.
The commit message mentions keeping all PHY parts powered up to avoid data
stream interruptions, but could this intermediate state mapping negate that
benefit during initial connection?
> }
>
> }
[Severity: High]
This is a pre-existing issue, but does the devres registration ordering in
rk_udphy_probe() cause a use-after-free of phy_u3 during unbind?
The Type-C orientation switch and mux are registered with devres before
the PHYs are created:
rk_udphy_probe() {
...
if (device_property_present(dev, "mode-switch")) {
ret = rk_udphy_setup_typec_mux(udphy);
...
udphy->phy_u3 = devm_phy_create(dev, dev->of_node, &rk_udphy_usb3_phy_ops);
...
}
During unbind, devres executes cleanup in reverse order. The PHY structures
are freed while the Type-C mux remains registered.
If a concurrent Type-C event arrives and calls rk_udphy_typec_mux_set(),
it invokes rk_udphy_update_power_state() which uses the freed pointer:
rk_udphy_update_power_state() {
...
phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_PRE_RESET);
...
}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-rockchip-usbdp-cleanup-v10-0-a392711ca8a9@collabora.com?part=36
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v10 30/36] usb: dwc3: core: support PHY reset notifications
From: sashiko-bot @ 2026-07-03 23:59 UTC (permalink / raw)
To: Sebastian Reichel
Cc: olteanv, conor+dt, robh, vkoul, neil.armstrong, linux-phy,
devicetree
In-Reply-To: <20260703-rockchip-usbdp-cleanup-v10-30-a392711ca8a9@collabora.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing error handling for pm_runtime_get_sync() in dwc3_usb3_phy_notify() can lead to hardware access on a suspended device and PM usage count leak.
- [High] AB-BA deadlock between PHY notifier rwsem and Runtime PM state lock during DWC3 runtime suspend.
- [High] Concurrent read-modify-write on DWC3_GUSB3PIPECTL can silently clear the newly added PHYSOFTRST bit.
- [Medium] The PM runtime usage counter is permanently leaked if the DWC3 driver unbinds mid-reset.
--
commit 7addd6da511b4b59c9d034fe7f0878d5304dda87
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
usb: dwc3: core: support PHY reset notifications
On recent Rockchip platforms (at least RK3588 & RK3576), DWC3 IP is used
with a USBDP PHY providing USB3 and DP. This PHY needs to be reset when
the mode changes, which may happen when plugging in different USB-C
devices.
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 517aa7f1486da..899decbd0da04 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
[ ... ]
> @@ -880,10 +886,78 @@ static int dwc3_phy_init(struct dwc3 *dwc)
> return ret;
> }
>
> +static int dwc3_usb3_phy_notify(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct dwc3_phy_nb *pnb = container_of(nb, struct dwc3_phy_nb, nb);
> + struct dwc3 *dwc = pnb->dwc;
> + int port = pnb->port_index;
> + unsigned long flags;
> + u32 reg;
> +
> + switch (action) {
> + case PHY_NOTIFY_PRE_RESET:
> + pm_runtime_get_sync(dwc->dev);
[Severity: High]
Does this code handle the case where pm_runtime_get_sync() fails?
If pm_runtime_get_sync() returns a negative error code (failing to resume the
device), it still increments the PM usage counter. Since the return value is
ignored, the code will proceed to unconditionally access DWC3 hardware
registers via dwc3_readl() and dwc3_writel(). Accessing registers on a
suspended, clock-gated device can trigger a synchronous external abort
(SError) or bus fault, causing a kernel panic.
Should this error path balance the usage counter via pm_runtime_put_noidle()
and return an error to prevent the leak and crash?
[Severity: High]
Can this pm_runtime_get_sync() call cause a deadlock during runtime suspend?
When a PHY reset notification arrives, the blocking_notifier_call_chain()
holds phy->notifier.rwsem (read lock), and dwc3_usb3_phy_notify() calls
pm_runtime_get_sync().
If DWC3 is concurrently runtime-suspending (in DEVICE mode, the state is
RPM_SUSPENDING), pm_runtime_get_sync() blocks waiting for the suspend to
finish.
The suspend thread calls dwc3_suspend_common() -> dwc3_core_exit() ->
dwc3_phy_unregister_notifiers(), which attempts to acquire
phy->notifier.rwsem (write lock) via blocking_notifier_chain_unregister():
dwc3_phy_unregister_notifiers()
phy_unregister_notifier()
blocking_notifier_chain_unregister() -> blocks on write lock
The two threads would mutually block, causing a strict AB-BA system deadlock.
> +
> + /*
> + * Assert USB3 PHY soft reset within DWC3 before the external
> + * PHY resets. This disconnects the PIPE interface, preventing
> + * the DWC3 from interfering with PHY reinitialization and
> + * avoiding LCPLL lock failures.
> + */
> + spin_lock_irqsave(&dwc->lock, flags);
> + reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(port));
> + reg |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
> + dwc3_writel(dwc, DWC3_GUSB3PIPECTL(port), reg);
> + spin_unlock_irqrestore(&dwc->lock, flags);
[Severity: High]
Can the PHYSOFTRST bit be silently cleared by concurrent register accesses?
While dwc3_usb3_phy_notify() modifies DWC3_GUSB3PIPECTL holding dwc->lock,
the existing dwc3_enable_susphy() function also modifies this register using a
read-modify-write sequence, but without holding dwc->lock:
dwc3_enable_susphy() {
...
reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(i));
if (enable && !dwc->dis_u3_susphy_quirk)
reg |= DWC3_GUSB3PIPECTL_SUSPHY;
else
reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
dwc3_writel(dwc, DWC3_GUSB3PIPECTL(i), reg);
...
}
If dwc3_enable_susphy() executes concurrently (e.g., triggered via
__dwc3_set_mode() during a role switch when the PHY reset is also triggered),
it can read the register before PHYSOFTRST is set and write it back after,
inadvertently clearing the bit.
> + break;
> +
> + case PHY_NOTIFY_POST_RESET:
[ ... ]
> + break;
> + }
> +
> + return NOTIFY_OK;
> +}
[ ... ]
> +static void dwc3_phy_unregister_notifiers(struct dwc3 *dwc)
> +{
> + int i;
> +
> + for (i = 0; i < dwc->num_usb3_ports; i++)
> + phy_unregister_notifier(dwc->usb3_generic_phy[i],
> + &dwc->usb3_phy_nb[i].nb);
> +}
[Severity: Medium]
Can this cause a PM runtime usage counter leak if the driver unbinds mid-reset?
PHY_NOTIFY_PRE_RESET increments the PM usage counter via pm_runtime_get_sync().
If the driver is unbound (dwc3_remove() -> dwc3_core_remove() ->
dwc3_core_exit()) before PHY_NOTIFY_POST_RESET is sent,
dwc3_phy_unregister_notifiers() cleanly unregisters the notifier.
Because the notifier is unregistered, the DWC3 driver never receives the
POST_RESET event, skipping the corresponding pm_runtime_put_autosuspend(). The
PM usage counter would remain unbalanced, preventing the device instance from
ever suspending if rebound.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-rockchip-usbdp-cleanup-v10-0-a392711ca8a9@collabora.com?part=30
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v10 34/36] phy: rockchip: usbdp: Simplify power state handling
From: sashiko-bot @ 2026-07-03 23:58 UTC (permalink / raw)
To: Sebastian Reichel
Cc: olteanv, vkoul, neil.armstrong, linux-phy, robh, conor+dt,
devicetree
In-Reply-To: <20260703-rockchip-usbdp-cleanup-v10-34-a392711ca8a9@collabora.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing USB3 port disable in high-speed mode causes DWC3 SErrors on probe.
--
commit e0219fd5f4d784e78ff969af266e10196367be69
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
phy: rockchip: usbdp: Simplify power state handling
This commit simplifies power state handling by introducing a sw_mode field in
addition to the hw_mode field. This allows the PHY to track hardware-supported
modes, software-requested modes, and the actual hardware status.
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index e2cd72643a7d7..ca9418fab8f3a 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
[ ... ]
> @@ -1288,16 +1311,22 @@ static const struct phy_ops rk_udphy_dp_phy_ops = {
> static int rk_udphy_usb3_phy_init(struct phy *phy)
> {
> struct rk_udphy *udphy = phy_get_drvdata(phy);
> + int ret;
>
> guard(mutex)(&udphy->mutex);
>
> - /* DP only or high-speed, disable U3 port */
> - if (!(udphy->hw_mode & UDPHY_MODE_USB) || udphy->hs) {
> - rk_udphy_u3_port_disable(udphy, true);
> + if (udphy->hs)
> return 0;
[Severity: High]
Does returning early here without disabling the U3 port cause DWC3 SErrors
during probe in high-speed mode?
Previously, this path explicitly called rk_udphy_u3_port_disable(udphy, true)
before returning. By returning early now, we completely bypass
rk_udphy_update_power_state(), which leaves the U3 port enabled. When the
DWC3 controller is probed, this can leave it clocked by the unpowered PHY
instead of the safe UTMI fallback clock, resulting in an SError.
> - }
>
> - return rk_udphy_power_on(udphy, UDPHY_MODE_USB);
> + udphy->sw_mode |= UDPHY_MODE_USB;
> +
> + ret = rk_udphy_update_power_state(udphy);
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-rockchip-usbdp-cleanup-v10-0-a392711ca8a9@collabora.com?part=34
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v10 19/36] phy: rockchip: usbdp: Re-init the PHY on orientation change
From: sashiko-bot @ 2026-07-03 23:51 UTC (permalink / raw)
To: Sebastian Reichel
Cc: linux-phy, robh, devicetree, vkoul, conor+dt, olteanv,
neil.armstrong
In-Reply-To: <20260703-rockchip-usbdp-cleanup-v10-19-a392711ca8a9@collabora.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch defers PHY hardware re-initialization during Type-C orientation changes by setting a `phy_needs_reinit` flag, assuming a subsequent call to `rk_udphy_update_power_state()` will apply the change. However, for USB3-only connections, this subsequent call is often never made, resulting in broken USB3 functionality.
--
commit af712ecc489de5c647a8e305c7beca4b1c6dd5cb
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
phy: rockchip: usbdp: Re-init the PHY on orientation change
This patch updates the rockchip usbdp PHY driver to track orientation
changes by checking if the cable orientation is reversed. If the orientation
changes, it flags the PHY as needing re-initialization so that the lane
muxing can be reconfigured.
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index e44d19c9d1193..4b454798c4ed4 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -620,6 +620,7 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
> enum typec_orientation orien)
> {
> struct rk_udphy *udphy = typec_switch_get_drvdata(sw);
> + bool flipped = orien == TYPEC_ORIENTATION_REVERSE;
>
> mutex_lock(&udphy->mutex);
>
> @@ -631,7 +632,10 @@ static int rk_udphy_orien_sw_set(struct typec_switch_dev *sw,
> goto unlock_ret;
> }
>
> - udphy->flip = orien == TYPEC_ORIENTATION_REVERSE;
> + if (udphy->flip != flipped)
> + udphy->phy_needs_reinit = true;
> +
[Severity: High]
Does this deferral of PHY initialization break USB3-only connections?
By only setting the phy_needs_reinit flag here, the actual hardware update
appears to be deferred until rk_udphy_update_power_state() is called to
consume it.
For USB3-only connections (especially on fixed-role ports or boards without a
mode-switch property), is rk_udphy_update_power_state() guaranteed to be
called after this orientation change?
If it isn't called, it seems a reverse-plugged USB3 device might fail to
connect because the reverse lane routing is never written to the hardware.
Additionally, if a DisplayPort device (which does trigger a power state
update) is plugged in reverse and then replaced with a normal USB3 device,
could the software state reset to normal while the hardware remains stuck
in reverse?
> + udphy->flip = flipped;
> rk_udphy_set_typec_default_mapping(udphy);
> rk_udphy_usb_bvalid_enable(udphy, true);
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-rockchip-usbdp-cleanup-v10-0-a392711ca8a9@collabora.com?part=19
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v10 20/36] phy: rockchip: usbdp: Factor out lane_mux_sel setup
From: sashiko-bot @ 2026-07-03 23:51 UTC (permalink / raw)
To: Sebastian Reichel
Cc: conor+dt, vkoul, robh, olteanv, neil.armstrong, linux-phy,
devicetree
In-Reply-To: <20260703-rockchip-usbdp-cleanup-v10-20-a392711ca8a9@collabora.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Overwriting udphy->dp_lanes in rk_udphy_set_typec_state destroys the hardware lane limit, leading to uninitialized array access.
--
commit b5a0ae84556b392b692f17a4e792243cb346f579
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
phy: rockchip: usbdp: Factor out lane_mux_sel setup
This commit factors out the Type-C state logic into a new helper function
rk_udphy_set_typec_state() to reduce code duplication when configuring the
USB+DP lane_mux_sel.
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index 4b454798c4ed4..4e54020d17551 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -582,6 +582,42 @@ static void rk_udphy_mode_set(struct rk_udphy *udphy, u8 mode)
> udphy->mode = mode;
> }
>
> +static void rk_udphy_set_typec_state(struct rk_udphy *udphy, unsigned long state)
> +{
> + u8 mode;
> +
> + switch (state) {
> + case TYPEC_DP_STATE_C:
> + case TYPEC_DP_STATE_E:
> + udphy->lane_mux_sel[0] = PHY_LANE_MUX_DP;
> + udphy->lane_mux_sel[1] = PHY_LANE_MUX_DP;
> + udphy->lane_mux_sel[2] = PHY_LANE_MUX_DP;
> + udphy->lane_mux_sel[3] = PHY_LANE_MUX_DP;
> + mode = UDPHY_MODE_DP;
> + udphy->dp_lanes = 4;
^^^^^^^^^^^^^^^^^^^
[Severity: High]
This is a pre-existing issue, but does overwriting udphy->dp_lanes here
destroy the hardware lane limit?
The variable udphy->dp_lanes seems to be overloaded. It is initially
populated from the device tree to represent the physical hardware lane
limit (e.g., 2 lanes). However, it is unconditionally overwritten here
to represent the requested Type-C state.
If a 2-lane PHY receives a 4-lane Type-C request, udphy->dp_lanes becomes 4.
Does this defeat the hardware limit check if (dp->lanes > udphy->dp_lanes)
in rk_udphy_dp_phy_verify_lanes() (added in a subsequent patch in this
series)?
If the hardware limit check is defeated, the DP controller might configure
4 lanes, causing rk_udphy_dp_phy_configure() to iterate 4 times and access
uninitialized elements in the dp_lane_sel array:
drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_configure()
...
if (dp->set_voltages) {
for (i = 0; i < udphy->lanes; i++) {
lane = udphy->dp_lane_sel[i];
...
Since dp_lane_sel[2] and dp_lane_sel[3] would be 0 (due to devm_kzalloc),
could this result in lane 0 being misconfigured multiple times, overriding
its intended settings and corrupting the USB3 state?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-rockchip-usbdp-cleanup-v10-0-a392711ca8a9@collabora.com?part=20
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v10 23/36] phy: rockchip: usbdp: Clear USB status on PHY exit
From: sashiko-bot @ 2026-07-03 23:49 UTC (permalink / raw)
To: Sebastian Reichel
Cc: devicetree, vkoul, robh, neil.armstrong, linux-phy, olteanv,
conor+dt
In-Reply-To: <20260703-rockchip-usbdp-cleanup-v10-23-a392711ca8a9@collabora.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Clearing `udphy->status` directly without checking if the PHY hardware should be disabled causes an unbounded clock reference count leak.
--
commit e836c3d9b92996113cb8593f93bf48152ed5de6c
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
phy: rockchip: usbdp: Clear USB status on PHY exit
This commit ensures the USB status flag is cleared when the USB3 PHY is
exited while the system is in DP-only mode. This happens if the USB3
controller device is unbound while a DP-only adapter is plugged in.
Closes: https://sashiko.dev/#/message/20260625-rockchip-usbdp-cleanup-v7-24-38eb3cf654fd%40collabora.com
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1273,8 +1273,10 @@ static int rk_udphy_usb3_phy_exit(struct phy *phy)
> guard(mutex)(&udphy->mutex);
>
> /* DP only or high-speed */
> - if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs)
> + if (!(udphy->mode & UDPHY_MODE_USB) || udphy->hs) {
> + udphy->status &= ~UDPHY_MODE_USB;
> return 0;
> + }
>
> rk_udphy_power_off(udphy, UDPHY_MODE_USB);
[Severity: High]
Does clearing udphy->status without actually disabling the hardware cause an
unbounded clock reference count leak?
When the system switches to DP-only mode, udphy->mode loses the
UDPHY_MODE_USB bit. If the USB controller is then unbound, we hit this path
and clear UDPHY_MODE_USB from udphy->status, but return early. The hardware
clocks remain enabled since we bypass the hardware disable call.
If the system then switches back to USB mode, rk_udphy_usb3_phy_init() will
see udphy->status as UDPHY_MODE_NONE and call rk_udphy_setup(). This calls
clk_bulk_prepare_enable() again, incrementing the clock refcount indefinitely
on every cycle.
I see this issue is addressed later in the series by commit
e0219fd5f4d784e78ff969af266e10196367be69 ("phy: rockchip: usbdp: Simplify
power state handling"), but doesn't this patch introduce a regression here
that breaks bisectability?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-rockchip-usbdp-cleanup-v10-0-a392711ca8a9@collabora.com?part=23
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v10 13/36] phy: rockchip: usbdp: Rename DP lane functions
From: sashiko-bot @ 2026-07-03 23:46 UTC (permalink / raw)
To: Sebastian Reichel
Cc: olteanv, vkoul, robh, neil.armstrong, conor+dt, devicetree,
linux-phy
In-Reply-To: <20260703-rockchip-usbdp-cleanup-v10-13-a392711ca8a9@collabora.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Failure during PHY initialization destroys active PHY hardware state while leaving software state out of sync.
--
commit 27edcd7e836a7f62ad5d0b336e4f8cfd4fedc67c
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
phy: rockchip: usbdp: Rename DP lane functions
The patch updates the two DisplayPort lane functions to follow the common
rk_udphy_dp_ prefix scheme for consistency.
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index b172ce6a722ee..7fb864a1486c0 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
[ ... ]
> @@ -1069,7 +1069,7 @@ static int rk_udphy_dp_phy_power_off(struct phy *phy)
> struct rk_udphy *udphy = phy_get_drvdata(phy);
>
> mutex_lock(&udphy->mutex);
> - rk_udphy_dplane_enable(udphy, 0);
> + rk_udphy_dp_lane_enable(udphy, 0);
> rk_udphy_power_off(udphy, UDPHY_MODE_DP);
> mutex_unlock(&udphy->mutex);
[Severity: High]
This isn't a bug introduced by this patch, but looking at the driver, does
failure during PHY initialization destroy active PHY hardware state while
leaving the software state out of sync?
If rk_udphy_init() fails during a mode change (e.g., PLL lock timeout when a
DP cable is plugged in), it aborts, asserts all PHY resets, and returns an
error:
drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_init() {
...
assert_resets:
rk_udphy_reset_assert_all(udphy);
return ret;
}
When this happens, rk_udphy_update_power_state() receives the error, notifies
DWC3 that the reset is complete, and returns early without clearing
udphy->status:
drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_update_power_state() {
...
ret = rk_udphy_init(udphy);
if (ret) {
phy_notify_reset(udphy->phy_u3, PHY_NOTIFY_POST_RESET);
return ret;
}
...
}
Will this cause the DWC3 USB controller to access a physically dead (reset)
PHY and crash the system with a synchronous external abort (SError), since
the controller is explicitly told the PHY is ready and the software driver is
left believing it is fully functional?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-rockchip-usbdp-cleanup-v10-0-a392711ca8a9@collabora.com?part=13
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v10 08/36] phy: rockchip: usbdp: Amend SSC modulation deviation
From: sashiko-bot @ 2026-07-03 23:44 UTC (permalink / raw)
To: Sebastian Reichel
Cc: neil.armstrong, conor+dt, devicetree, olteanv, linux-phy, vkoul,
robh
In-Reply-To: <20260703-rockchip-usbdp-cleanup-v10-8-a392711ca8a9@collabora.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Medium] Partial state update during the configuration verification phase leaves inconsistent driver state on error.
--
commit 90f0c5bc1613dc3c61ed76de74f75f85b147f759
Author: Frank Wang <frank.wang@rock-chips.com>
phy: rockchip: usbdp: Amend SSC modulation deviation
Move SSC modulation deviation into private config of clock.
- 24M: 0x00d4[5:0] = 0x30
- 26M: 0x00d4[5:0] = 0x33
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index e3f5a26c876a5..cb6acadfbccfb 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
[ ... ]
While reviewing this, I noticed a pre-existing issue in this file:
drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_configure() {
...
if (dp->set_rate) {
ret = rk_udphy_dp_phy_verify_link_rate(udphy, dp);
if (ret)
return ret;
}
if (dp->set_lanes) {
ret = rk_udphy_dp_phy_verify_lanes(udphy, dp);
if (ret)
return ret;
}
if (dp->set_voltages) {
ret = rk_udphy_dp_phy_verify_voltages(udphy, dp);
if (ret)
return ret;
}
...
}
[Severity: Medium]
This isn't a bug introduced by this patch, but does partial state update
during the configuration verification phase leave inconsistent driver state
on error?
The helpers rk_udphy_dp_phy_verify_link_rate() and
rk_udphy_dp_phy_verify_lanes() prematurely mutate the driver's internal
state before guaranteeing overall configuration success.
If a caller invokes phy_configure() with set_rate=1 and set_voltages=1,
the verify_link_rate check might pass and immediately overwrite
udphy->link_rate.
If the verify_voltages check then fails, the function aborts without
programming the hardware or restoring the old link_rate.
Could a subsequent call setting only voltages then use the corrupted
link_rate to configure the PHY clock inversion registers?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-rockchip-usbdp-cleanup-v10-0-a392711ca8a9@collabora.com?part=8
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v10 01/36] dt-bindings: phy: rockchip-usbdp: add improved ports scheme
From: sashiko-bot @ 2026-07-03 23:42 UTC (permalink / raw)
To: Sebastian Reichel
Cc: devicetree, robh, neil.armstrong, olteanv, vkoul, linux-phy,
conor+dt
In-Reply-To: <20260703-rockchip-usbdp-cleanup-v10-1-a392711ca8a9@collabora.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Typo in the deprecation keyword ('depreated' instead of 'deprecated').
--
commit 80cbcd5498584cd573a979a5b4e236da93d09776
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
dt-bindings: phy: rockchip-usbdp: add improved ports scheme
This commit introduces a proper port scheme to the Rockchip USBDP PHY binding
to support USB-C DP AltMode, replacing an undocumented scheme. It deprecates
the single port node in favor of a ports node containing multiple specific
endpoints. The new design roughly follows the Qualcomm QMP USB4-USB3-DP PHY
controller binding while adding an additional port for the USB-C SBU mux.
> diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-usbdp.yaml b/Documentation/devicetree/bindings/phy/phy-rockchip-usbdp.yaml
> index 8b7059d5b1826..35d6717b9cc9e 100644
> --- a/Documentation/devicetree/bindings/phy/phy-rockchip-usbdp.yaml
> +++ b/Documentation/devicetree/bindings/phy/phy-rockchip-usbdp.yaml
> @@ -110,10 +110,34 @@ properties:
>
> port:
> $ref: /schemas/graph.yaml#/properties/port
> + depreated: true
^^^^^^^^^
[Severity: Low]
This isn't a bug, but there is a typo here ("depreated" instead of
"deprecated").
> description:
> A port node to link the PHY to a TypeC controller for the purpose of
> handling orientation switching.
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-rockchip-usbdp-cleanup-v10-0-a392711ca8a9@collabora.com?part=1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH v10 27/36] phy: rockchip: usbdp: Handle rk_udphy_reset_deassert errors
From: Sebastian Reichel @ 2026-07-03 18:13 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Heiko Stuebner, Frank Wang,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Thinh Nguyen,
Greg Kroah-Hartman, Philipp Zabel
Cc: Andy Yan, Dmitry Baryshkov, Yubing Zhang, Alexey Charkov,
linux-phy, linux-arm-kernel, linux-rockchip, linux-kernel, kernel,
devicetree, linux-usb, Sebastian Reichel, Sashiko
In-Reply-To: <20260703-rockchip-usbdp-cleanup-v10-0-a392711ca8a9@collabora.com>
Handle rk_udphy_reset_deassert returning errors to avoid theoretical
(Rockchip reset controller driver does not return errors) SError.
Fixes: 2f70bbddeb45 ("phy: rockchip: add usbdp combo phy driver")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/message/20260626211151.2332F1F000E9%40smtp.kernel.org
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
drivers/phy/rockchip/phy-rockchip-usbdp.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
index 9e77513643a3..c645fea67c4d 100644
--- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
+++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
@@ -803,8 +803,12 @@ static int rk_udphy_init(struct rk_udphy *udphy)
/* Step 1: power on pma and deassert apb rstn */
rk_udphy_grfreg_write(udphy->udphygrf, &cfg->grfcfg.low_pwrn, true);
- rk_udphy_reset_deassert(udphy, "pma_apb");
- rk_udphy_reset_deassert(udphy, "pcs_apb");
+ ret = rk_udphy_reset_deassert(udphy, "pma_apb");
+ if (ret)
+ goto assert_resets;
+ ret = rk_udphy_reset_deassert(udphy, "pcs_apb");
+ if (ret)
+ goto assert_resets;
/* Step 2: set init sequence and phy refclk */
ret = regmap_multi_reg_write(udphy->pma_regmap, rk_udphy_init_sequence,
@@ -830,8 +834,11 @@ static int rk_udphy_init(struct rk_udphy *udphy)
FIELD_PREP(CMN_DP_LANE_EN_ALL, 0));
/* Step 4: deassert init rstn and wait for 200ns from datasheet */
- if (udphy->mode & UDPHY_MODE_USB)
- rk_udphy_reset_deassert(udphy, "init");
+ if (udphy->mode & UDPHY_MODE_USB) {
+ ret = rk_udphy_reset_deassert(udphy, "init");
+ if (ret)
+ goto assert_resets;
+ }
if (udphy->mode & UDPHY_MODE_DP) {
regmap_update_bits(udphy->pma_regmap, CMN_DP_RSTN_OFFSET,
@@ -843,8 +850,14 @@ static int rk_udphy_init(struct rk_udphy *udphy)
/* Step 5: deassert cmn/lane rstn */
if (udphy->mode & UDPHY_MODE_USB) {
- rk_udphy_reset_deassert(udphy, "cmn");
- rk_udphy_reset_deassert(udphy, "lane");
+ ret = rk_udphy_reset_deassert(udphy, "cmn");
+ if (ret)
+ goto assert_resets;
+
+ ret = rk_udphy_reset_deassert(udphy, "lane");
+ if (ret)
+ goto assert_resets;
+
}
/* Step 6: wait for lock done of pll */
--
2.53.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox