* Re: [RFC PATCH 08/10] net: stmmac: dwmac-rk: add SGMII support for RK3568
From: Coia Prant @ 2026-07-15 20:33 UTC (permalink / raw)
To: sashiko-reviews
Cc: robh, devicetree, neil.armstrong, olteanv, linux-phy, vkoul,
conor+dt
In-Reply-To: <20260715191455.D033A1F00ACA@smtp.kernel.org>
> [ ... ]
> > @@ -1379,6 +1441,9 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
> > if (ret)
> > return ret;
> >
> > + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
> > + goto set_mode;
> > +
>
> [Severity: High]
> Does this goto skip enabling the MAC clocks? By jumping directly to set_mode,
> we bypass the call to gmac_clk_enable() further down in this function.
>
> Can this cause a synchronous external abort when the MAC registers are accessed
> subsequently while their clock domain remains disabled?
You are correct. The original RFC patch had this issue: the SGMII path
bypassed `gmac_clk_enable()` via the `goto set_mode`, which could lead
to hardware faults when accessing MAC registers with the clock disabled.
This has been fixed in the updated version of the patch. The clock enable
is now moved before the SGMII check:
ret = gmac_clk_enable(bsp_priv, true);
if (ret)
return ret;
if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
goto set_mode;
So the clocks are always enabled before any register access, regardless of
the interface mode.
The error handling has also been cleaned up with a unified `clk_disable`
label, and the default (unhandled interface) case now properly returns an
error and disables the clocks.
Regarding stable backport: this issue does not exist in the 6.18
kernel that I am using for backport testing, as the code structure in
`rk_gmac_powerup()` is significantly different there. The fix only
affects the mainline version of the patch.
Thanks,
Coia
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [RFC PATCH 06/10] net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568
From: Coia Prant @ 2026-07-15 20:19 UTC (permalink / raw)
To: sashiko-reviews
Cc: vkoul, olteanv, neil.armstrong, robh, devicetree, conor+dt,
linux-phy
In-Reply-To: <20260715191453.DEE731F000E9@smtp.kernel.org>
Thanks for the review.
> [ ... ]
>
> > @@ -884,7 +889,7 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
> >
> > if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> > ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
> > - mdio_ctrl | BMCR_ANENABLE);
> > + mdio_ctrl | BMCR_ANENABLE | BMCR_ANRESTART);
>
> [Severity: Medium]
> The commit message mentions improving SGMII AN state handling, but this
> adds BMCR_ANRESTART to the 1000BASE-X configuration path in
> xpcs_config_aneg_c37_1000basex(). Was this unintentional, or should the
> commit message be updated to mention this behavior change for 1000BASE-X?
You are correct. This was unintentional. The `BMCR_ANRESTART` change in
`xpcs_config_aneg_c37_1000basex()` is a mistake that was introduced
during development and should not be there. I will remove it in v2.
The SGMII path (`xpcs_config_aneg_c37_sgmii()`) is the one that actually
needs this change, and that is what I have tested on the Photonicat board.
The 1000BASE-X path was not tested and the change was not intended.
> [ ... ]
>
> > @@ -1086,7 +1094,24 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
> > state->duplex = DUPLEX_FULL;
> > else
> > state->duplex = DUPLEX_HALF;
> > - } else if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
> > +
> > + return 0;
> > + }
> > +
> > + /* Clear AN complete status or interrupt */
> > + if (state->an_complete)
> > + xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
>
> [Severity: High]
> Can clearing the interrupt here lead to a permanently stalled link on
> Wangxun platforms if the subsequent xpcs_read() fails?
>
> The original code cleared DW_VR_MII_AN_INTR_STS at the very end of the
> DW_VR_MII_AN_STS_C37_ANCMPLT_INTR block, after successfully reading
> MII_BMCR and MII_ADVERTISE.
>
> By moving the interrupt clear here:
>
> if (state->an_complete)
> xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
>
> If a transient MDIO error occurs during the subsequent MII_BMCR read:
>
> speed = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
> if (speed < 0)
> return speed;
>
> The function will exit with an error and the link will remain down, but the
> AN complete status has already been cleared. Will this prevent the Wangxun
> initialization block from running on the next poll and leave the link
> stuck?
No, this is safe. The `ret` value used to determine whether to enter the
Wangxun-specific path is read from the register before the clear operation.
The clear only affects the hardware register, not the stack variable. So
Wangxun's `if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR)` block will still
be reached correctly.
If the subsequent MDIO read fails, the function will return the error and
the link state will remain down. Since the hardware interrupt status has
already been cleared, the next poll will not see `state->an_complete` set
again, so the Wangxun block will not be entered until a new AN event
occurs. This is actually the intended behavior — the interrupt is only
valid for the current AN event and should not be processed more than once.
Therefore, the change does not introduce a regression on Wangxun platforms.
I will remove the 1000BASE-X change in v2 and keep only the SGMII-related
fixes.
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [RFC PATCH 05/10] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes
From: Coia Prant @ 2026-07-15 20:09 UTC (permalink / raw)
To: sashiko-reviews
Cc: robh, devicetree, neil.armstrong, linux-phy, vkoul, olteanv,
conor+dt
In-Reply-To: <20260715191452.4A4821F00A3F@smtp.kernel.org>
Thanks for the review.
> [Severity: Low]
> The commit message states these clocks are used as assigned-clock-parents
> for the respective GMAC nodes, but the patch doesn't appear to add the
> assigned-clock-parents properties to the gmac0 or gmac1 nodes. Are these
> assignments missing?
The `assigned-clock-parents` assignments are not added in the SoC-level
`rk3568.dtsi` because doing so would break existing boards that use
RGMII mode. The clock parent for GMAC0/1 is currently hardcoded to the
RGMII clock path in the mainline device tree.
Instead, the `assigned-clock-parents` are applied at the board level.
This is already done in Patch 9 of this series for the Photonicat board:
&gmac0 {
assigned-clocks = <&cru SCLK_GMAC0_RX_TX>;
assigned-clock-parents = <&xpcs_gmac0_clk>;
...
};
So the assignments exist, but in the board DTS rather than the SoC DTSI.
> [Severity: Low]
> Should this internal SoC clock be modeled as a clock provider from the
> generating XPCS IP node using #clock-cells, rather than using a dummy
> fixed-clock?
The XPCS block does not have a clock controller interface. It simply
outputs a fixed 125MHz clock for the GMAC when operating in SGMII mode.
There is no register interface to control or configure this clock — it
is a fixed-rate output from the PCS hardware.
Using `fixed-clock` is therefore the correct and accurate representation
of the hardware. The XPCS is not a clock controller, so it should not
be modeled as one.
> Additionally, the commit message states "All nodes are left disabled by
> default", but this node is missing the status = "disabled" property. Will
> this leave the clock unconditionally enabled?
> [Severity: Low]
>
> Similarly, does this node also need a status = "disabled" property to align
> with the behavior described in the commit message?
The fixed-clock nodes are always present and do not have a `status`
property. This is intentional: they are part of the SoC's fixed clock
infrastructure, similar to other fixed-clock nodes in the SoC DTSI.
They are not "devices" that need to be enabled/disabled, but rather
static clock providers.
The "all nodes are left disabled by default" statement in the commit
message refers to the XPCS node and its pcs-mii sub-nodes, not the
fixed-clock nodes. I will revise the commit message to clarify this.
Thanks,
Coia
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [RFC PATCH 04/10] dt-bindings: net: pcs: add rockchip,rk3568-xpcs binding
From: Coia Prant @ 2026-07-15 20:01 UTC (permalink / raw)
To: sashiko-reviews
Cc: conor+dt, devicetree, vkoul, neil.armstrong, olteanv, robh,
linux-phy
In-Reply-To: <20260715191451.515E01F00A3E@smtp.kernel.org>
> [ ... ]
>
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/rk3568-cru.h>
> > + #include <dt-bindings/power/rk3568-power.h>
> > +
> > + pcs@fda00000 {
> > + compatible = "rockchip,rk3568-xpcs";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + reg = <0x0 0xfda00000 0x0 0x200000>;
>
> [Severity: Low]
> Does this 64-bit register format fail dt_binding_check validation?
>
> The example is extracted into a dummy root node with default
> #address-cells = <1> and #size-cells = <1>. This means the 4 cells here will
> be parsed as two items, which violates the maxItems: 1 constraint defined
> in the schema for the reg property.
I will update the example to use
the correct format that passes `dt_binding_check`. The proper way to
represent this is:
reg = <0xfda00000 0x200000>;
or with explicit address/size cells:
reg = <0x0 0xfda00000 0x0 0x200000>;
I will adjust the example to match what `dt_binding_check` expects.
> [Severity: Low]
> Will this fail to compile during dt_binding_check?
>
> The example uses the PHY_TYPE_SGMII macro, but the required
> <dt-bindings/phy/phy.h> header isn't included in the example.
Yes, you are correct. I will add the missing include:
#include <dt-bindings/phy/phy.h>
to the example section.
Thanks for the review. I'll address both issues in v2.
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [RFC PATCH 03/10] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568
From: Coia Prant @ 2026-07-15 19:55 UTC (permalink / raw)
To: sashiko-reviews
Cc: linux-phy, neil.armstrong, olteanv, robh, conor+dt, devicetree,
vkoul
In-Reply-To: <20260715191450.5784D1F00A3D@smtp.kernel.org>
> [Severity: High]
> Does this break device tree backward compatibility?
>
> When the rockchip,sgmii-mac-sel property is missing from an older device
> tree, priv->sgmii_mac_sel defaults to 0 in rockchip_combphy_parse_dt().
>
> By unconditionally calling rockchip_combphy_param_write() here, the driver
> now actively clears the MAC selection bit to 0 (GMAC0).
>
> Because the hardware reset value is GMAC1, this actively overrides the default
> hardware and bootloader state that older device trees were relying on.
>
> Could the code check if the property was actually present before writing
> to the register, to ensure the hardware default is preserved for existing
> setups?
Thanks for the detailed review. Regarding the backward compatibility
concern for Patch 3:
First, the driver code in the RFC patch set has already been revised
to align with the hardware default: `priv->sgmii_mac_sel = 1` (GMAC1).
So when the property is omitted, the hardware default is preserved.
Second, and more importantly, there is no backward compatibility
issue in practice: SGMII support for RK3568 has never been functional
in mainline before this series. There are no existing device trees
that rely on this path, because the PHY driver simply did not support
SGMII mode at all. Older device trees either didn't enable SGMII,
or were never operational.
So this change only affects new users who explicitly enable SGMII
and set the property as needed. It does not break anything that
was previously working.
Thanks,
Coia
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [RFC PATCH 02/10] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property
From: Coia Prant @ 2026-07-15 19:53 UTC (permalink / raw)
To: sashiko-reviews
Cc: olteanv, devicetree, linux-phy, vkoul, robh, conor+dt,
neil.armstrong
In-Reply-To: <20260715191449.437E11F00A3A@smtp.kernel.org>
Thanks for the review. I'll address both issues in v2.
> --- Patch [2]: [RFC PATCH 02/10] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property ---
> commit 0d950b4055dd144fecf5059f16535f2c8d94b5d3
> Author: Coia Prant <coiaprant@gmail.com>
>
> dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property
>
> This commit adds the "rockchip,sgmii-mac-sel" property to the Rockchip
> Naneng combphy device tree binding. It allows selecting between GMAC0
> and GMAC1 for the SGMII interface on the RK3568 SoC.
>
> > This is necessary for boards such as the Ariaboard Photonicat, where
> > the SGMII interface is connected to GMAC0 and needs to be explicitly
> > configured.
>
> [Severity: Medium]
> Is it actually necessary to explicitly configure the property to 0 for
> GMAC0?
>
> The schema defines the default as 0, which implies that omitting the property
> would naturally default the hardware to GMAC0. If explicit configuration is
> required, does this mean the hardware or bootloader leaves the register at 1,
> making the `default: 0` documented in the schema incorrect?
Regarding the default value: the hardware reset value for the
`pipe_sgmii_mac_sel` bit is GMAC1 (1). I have updated the binding
to set `default: 1` and revised the commit message accordingly,
removing the contradictory "default 0" statement.
The default now matches the hardware reset value.
> [Severity: Low]
> Should this property be conditionally restricted to the RK3568 compatible
> string?
>
> Because it is defined in the top-level properties block, it will pass
> validation for all other hardware variants (like RK3528 or RK3588) that do
> not actually support SGMII. Could an `if-then` block in the `allOf` section
> be used to set this property to false for compatibles other than
> `rockchip,rk3568-naneng-combphy`?
Regarding the property scope: you are correct that this property
only makes sense for RK3568. I have added an `if-then` conditional
in the `allOf` section to restrict this property to
`rockchip,rk3568-naneng-combphy` only.
Thanks,
Coia
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [RFC PATCH 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver
From: sashiko-bot @ 2026-07-15 19:14 UTC (permalink / raw)
To: Coia Prant
Cc: vkoul, neil.armstrong, linux-phy, conor+dt, devicetree, robh,
olteanv
In-Reply-To: <20260714191341.690906-8-coiaprant@gmail.com>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Asymmetric MDIO read/write remapping for VEND2 control registers corrupts port 0 and breaks configuration for ports 1-3.
- [High] Accessing the supplier device's `drvdata` without synchronization before establishing the device link causes a Use-After-Free race condition.
- [Medium] Clock `csr_clk` leaks if the device is removed while its runtime PM state is active.
- [Medium] `xpcs_rk_create` extracts driver data from an arbitrary device node without validating its compatible string or bound driver, leading to type confusion.
- [Low] The Makefile modifications incorrectly split the composite `pcs_xpcs` module into multiple independent modules, causing `modpost` build failures.
- [Low] The `dev_pm_ops` structure is assigned unconditionally to the driver's `.pm` field, causing dead code when `CONFIG_PM` is disabled.
--
--- Patch [7]: [RFC PATCH 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver ---
commit c0159daf5cb4d2e6a0fbbe7a38525a8e230e6d93
Author: Coia Prant <coiaprant@gmail.com>
net: pcs: xpcs: add Rockchip RK3568 platform glue driver
The RK3568 SoC integrates a Synopsys DesignWare XPCS accessed via APB3
memory-mapped registers. This driver provides the glue logic to make the
XPCS accessible to the generic pcs-xpcs core.
Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf
> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
> index 4f7920618b900..c809b7f942a51 100644
> --- a/drivers/net/pcs/Makefile
> +++ b/drivers/net/pcs/Makefile
> @@ -1,10 +1,9 @@
> # SPDX-License-Identifier: GPL-2.0
> # Makefile for Linux PCS drivers
>
> -pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-plat.o \
> - pcs-xpcs-nxp.o pcs-xpcs-wx.o
> -
> -obj-$(CONFIG_PCS_XPCS) += pcs_xpcs.o
> +obj-$(CONFIG_PCS_XPCS) += pcs-xpcs.o pcs-xpcs-nxp.o pcs-xpcs-wx.o
[Severity: Low]
Does this change cause build failures when CONFIG_PCS_XPCS=m?
Replacing the composite module assignment (pcs_xpcs-y) with individual
object assignments causes Kbuild to build separate modules instead of one
composite module, which can lead to undefined references during modpost.
> +obj-$(CONFIG_PCS_XPCS_PLATFORM) += pcs-xpcs-plat.o
> +obj-$(CONFIG_PCS_XPCS_ROCKCHIP) += pcs-xpcs-rk.o
[ ... ]
> diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> new file mode 100644
> index 0000000000000..ed969a38d544d
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c
[ ... ]
> +static int xpcs_rk_mdio_read_remapping(int addr, int dev, int reg)
> +{
> + switch (dev) {
> + case MDIO_MMD_PMAPMD:
> + return ROCKCHIP_MMD_PMAPMD;
> + case MDIO_MMD_VEND2:
> + break;
> + default:
> + return -ENXIO;
> + }
> +
> + switch (addr) {
> + case 0:
> + return ROCKCHIP_MMD_MII;
> + case 1:
> + return ROCKCHIP_MMD_MII1;
[ ... ]
> +static int xpcs_rk_mdio_write_remapping(int addr, int dev, int reg)
> +{
> + switch (dev) {
> + case MDIO_MMD_PMAPMD:
> + return ROCKCHIP_MMD_PMAPMD;
> + case MDIO_MMD_VEND2:
> + break;
> + default:
> + return -ENXIO;
> + }
> +
> + /* Writable only on MII */
> + switch (reg) {
> + case DW_VR_MII_AN_CTRL:
> + case DW_VR_MII_AN_INTR_STS:
> + case DW_VR_MII_EEE_MCTRL0:
> + case DW_VR_MII_EEE_MCTRL1:
> + case DW_VR_MII_DIG_CTRL2:
> + return ROCKCHIP_MMD_MII;
> + default:
> + break;
> + }
[Severity: High]
Can this asymmetric remapping between reads and writes cause configuration
corruption on port 0?
In xpcs_rk_mdio_write_remapping(), writes to these VEND2 registers are
unconditionally mapped to ROCKCHIP_MMD_MII (port 0). However,
xpcs_rk_mdio_read_remapping() lacks an equivalent redirect for reads.
Because the generic XPCS driver uses read-modify-write operations on VEND2
control registers for ports 1-3, this will result in reading from the target
port's MMD but overwriting port 0's configuration.
> +
> + switch (addr) {
> + case 0:
[ ... ]
> +static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
> +{
> + struct device *dev = &pxpcs->pdev->dev;
> + int ret;
> +
> + pxpcs->csr_clk = devm_clk_get(dev, "csr");
> + if (IS_ERR(pxpcs->csr_clk))
> + return dev_err_probe(dev, PTR_ERR(pxpcs->csr_clk),
> + "Failed to get CSR clock\n");
> +
> + pm_runtime_set_suspended(dev);
> + ret = devm_pm_runtime_enable(dev);
[Severity: Medium]
Will this leak the csr_clk if the device is removed while active?
The generic devm_pm_runtime_enable() runs pm_runtime_disable() on cleanup,
which cancels pending suspends but does not synchronously suspend an active
device.
Since this driver lacks a .remove callback to manually suspend the device,
xpcs_rk_pm_runtime_suspend() might never be invoked on unbind, leaving the
clock prepare and enable counts unbalanced.
> + if (ret) {
> + dev_err(dev, "Failed to enable runtime-PM\n");
> + return ret;
> + }
[ ... ]
> +struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
> +{
[ ... ]
> + pdev = of_find_device_by_node(pcs_np);
> + of_node_put(pcs_np);
> + if (!pdev)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + pxpcs = platform_get_drvdata(pdev);
[Severity: Medium]
Could this blind cast lead to type confusion?
The code extracts driver data from the platform device associated with
pcs_np without validating if the device is actually compatible with
rockchip,rk3568-xpcs or bound to this driver.
If a misconfigured device tree points to a different type of device (like
an I2C controller), casting its drvdata to struct dw_xpcs_rk * and
dereferencing it could crash the system.
> + if (!pxpcs || !pxpcs->bus) {
[Severity: High]
Is there a use-after-free race condition when accessing drvdata here?
This checks and dereferences pxpcs->bus before establishing a device link
to lock the supplier.
If a user concurrently unbinds the XPCS supplier driver (for example, via
sysfs), devres will free the dw_xpcs_rk structure and clear the drvdata.
The consumer driver might then dereference the freed pxpcs->bus before
device_link_add() is called.
> + put_device(&pdev->dev);
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> + xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);
[ ... ]
> +static struct platform_driver xpcs_rk_driver = {
> + .probe = xpcs_rk_probe,
> + .driver = {
> + .name = "rk_xpcs-dwxpcs",
> + .pm = &xpcs_rk_pm_ops,
[Severity: Low]
Will this assignment cause dead code when CONFIG_PM is disabled?
Assigning the dev_pm_ops structure unconditionally to the .pm field without
the pm_ptr() wrapper results in dead code and possible compiler warnings
when CONFIG_PM is not set, which violates the PM subsystem guidelines.
> + .of_match_table = xpcs_rk_of_ids,
> + },
> +};
> +module_platform_driver(xpcs_rk_driver);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=7
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [RFC PATCH 08/10] net: stmmac: dwmac-rk: add SGMII support for RK3568
From: sashiko-bot @ 2026-07-15 19:14 UTC (permalink / raw)
To: Coia Prant
Cc: robh, devicetree, neil.armstrong, olteanv, linux-phy, vkoul,
conor+dt
In-Reply-To: <20260714191341.690906-9-coiaprant@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The SGMII initialization path inadvertently skips enabling the MAC clocks, leading to hardware hangs or faults when MAC registers are accessed.
--
--- Patch [8]: [RFC PATCH 08/10] net: stmmac: dwmac-rk: add SGMII support for RK3568 ---
commit 7be50db2ac8c40c8451407f345aaaaf2d0538799
Author: Coia Prant <coiaprant@gmail.com>
net: stmmac: dwmac-rk: add SGMII support for RK3568
The RK3568 SoC integrates a Synopsys DesignWare XPCS that can be connected
to GMAC0 or GMAC1 in SGMII mode. Add the necessary glue logic to support
this configuration.
Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 8d7042e689261..eca482b4b6bfc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
[ ... ]
> @@ -1379,6 +1441,9 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
> if (ret)
> return ret;
>
> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
> + goto set_mode;
> +
[Severity: High]
Does this goto skip enabling the MAC clocks? By jumping directly to set_mode,
we bypass the call to gmac_clk_enable() further down in this function.
Can this cause a synchronous external abort when the MAC registers are accessed
subsequently while their clock domain remains disabled?
> ret = rk_get_phy_intf_sel(bsp_priv->phy_iface);
> if (ret < 0)
> return ret;
[ ... ]
> - /*rmii or rgmii*/
> +set_mode:
> + /* rmii, rgmii, sgmii */
> switch (bsp_priv->phy_iface) {
> case PHY_INTERFACE_MODE_RGMII:
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=8
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [RFC PATCH 04/10] dt-bindings: net: pcs: add rockchip,rk3568-xpcs binding
From: sashiko-bot @ 2026-07-15 19:14 UTC (permalink / raw)
To: Coia Prant
Cc: conor+dt, devicetree, vkoul, neil.armstrong, olteanv, robh,
linux-phy
In-Reply-To: <20260714191341.690906-5-coiaprant@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The device tree example uses a 64-bit `reg` format that fails `dt_binding_check` validation due to a mismatch with the default `#address-cells`.
- [Low] The device tree example fails to include the header for `PHY_TYPE_SGMII`, causing a compilation error during `dt_binding_check`.
--
--- Patch [4]: [RFC PATCH 04/10] dt-bindings: net: pcs: add rockchip,rk3568-xpcs binding ---
commit 2d8d4cd3ced5ab201d6610a9b6b870ceb6ceaed5
Author: Coia Prant <coiaprant@gmail.com>
dt-bindings: net: pcs: add rockchip,rk3568-xpcs binding
This commit adds device tree binding documentation for the Synopsys DesignWare
XPCS integrated on the Rockchip RK3568 SoC, supporting 1000BASE-X, SGMII,
and QSGMII modes with four MII ports.
> diff --git a/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml b/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml
> new file mode 100644
> index 0000000000000..14fadf67c793a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml
[ ... ]
> +examples:
> + - |
> + #include <dt-bindings/clock/rk3568-cru.h>
> + #include <dt-bindings/power/rk3568-power.h>
> +
> + pcs@fda00000 {
> + compatible = "rockchip,rk3568-xpcs";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x0 0xfda00000 0x0 0x200000>;
[Severity: Low]
Does this 64-bit register format fail dt_binding_check validation?
The example is extracted into a dummy root node with default
#address-cells = <1> and #size-cells = <1>. This means the 4 cells here will
be parsed as two items, which violates the maxItems: 1 constraint defined
in the schema for the reg property.
> + clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
> + clock-names = "csr", "eee";
> + phys = <&combphy2 PHY_TYPE_SGMII>;
[Severity: Low]
Will this fail to compile during dt_binding_check?
The example uses the PHY_TYPE_SGMII macro, but the required
<dt-bindings/phy/phy.h> header isn't included in the example.
> + phy-names = "serdes";
> + power-domains = <&power RK3568_PD_PIPE>;
> +
> + pcs-mii@0 {
> + reg = <0>;
> + };
> + };
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@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: [RFC PATCH 06/10] net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568
From: sashiko-bot @ 2026-07-15 19:14 UTC (permalink / raw)
To: Coia Prant
Cc: vkoul, olteanv, neil.armstrong, robh, devicetree, conor+dt,
linux-phy
In-Reply-To: <20260714191341.690906-7-coiaprant@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The AN complete status is cleared before reading MII_BMCR in the Wangxun-specific path, which can cause the link to stall permanently if a subsequent MDIO read fails.
- [Medium] The patch modifies the 1000BASE-X configuration path to assert BMCR_ANRESTART without any mention or justification in the commit message.
--
--- Patch [6]: [RFC PATCH 06/10] net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568 ---
commit 8c1976e4f52a5e7d8b4bab114f6cf214fe50a249
Author: Coia Prant <coiaprant@gmail.com>
net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568
This patch modifies SGMII AN state handling for Rockchip RK3568 by checking
CL37_ANSGM_STS for link status. It clears the AN complete interrupt and
restarts AN for non-Wangxun platforms if complete, and clears CL37 AN
complete status before starting AN to ensure a clean initial state.
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index e69fa2f0a0e8d..cf370ba247cac 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
[ ... ]
> @@ -884,7 +889,7 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
>
> if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
> - mdio_ctrl | BMCR_ANENABLE);
> + mdio_ctrl | BMCR_ANENABLE | BMCR_ANRESTART);
[Severity: Medium]
The commit message mentions improving SGMII AN state handling, but this
adds BMCR_ANRESTART to the 1000BASE-X configuration path in
xpcs_config_aneg_c37_1000basex(). Was this unintentional, or should the
commit message be updated to mention this behavior change for 1000BASE-X?
> if (ret < 0)
> return ret;
> }
[ ... ]
> @@ -1086,7 +1094,24 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
> state->duplex = DUPLEX_FULL;
> else
> state->duplex = DUPLEX_HALF;
> - } else if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
> +
> + return 0;
> + }
> +
> + /* Clear AN complete status or interrupt */
> + if (state->an_complete)
> + xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
[Severity: High]
Can clearing the interrupt here lead to a permanently stalled link on
Wangxun platforms if the subsequent xpcs_read() fails?
The original code cleared DW_VR_MII_AN_INTR_STS at the very end of the
DW_VR_MII_AN_STS_C37_ANCMPLT_INTR block, after successfully reading
MII_BMCR and MII_ADVERTISE.
By moving the interrupt clear here:
if (state->an_complete)
xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
If a transient MDIO error occurs during the subsequent MII_BMCR read:
speed = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
if (speed < 0)
return speed;
The function will exit with an error and the link will remain down, but the
AN complete status has already been cleared. Will this prevent the Wangxun
initialization block from running on the next poll and leave the link
stuck?
> +
> + if (xpcs->info.pma != WX_TXGBE_XPCS_PMA_10G_ID) {
> + /* If the link down, restart Auto-Negotiation */
> + if (state->an_complete)
> + xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
> + BMCR_ANRESTART);
> +
> + return 0;
> + }
> +
> + if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
> int speed, duplex;
>
> state->link = true;
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=6
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [RFC PATCH 03/10] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568
From: sashiko-bot @ 2026-07-15 19:14 UTC (permalink / raw)
To: Coia Prant
Cc: linux-phy, neil.armstrong, olteanv, robh, conor+dt, devicetree,
vkoul
In-Reply-To: <20260714191341.690906-4-coiaprant@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch breaks device tree backward compatibility by forcing the SGMII MAC selection to GMAC0 when the optional `rockchip,sgmii-mac-sel` property is absent, overriding the hardware default.
--
--- Patch [3]: [RFC PATCH 03/10] phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568 ---
commit 605f86936fd231092ab8489f15df507702255912
Author: Coia Prant <coiaprant@gmail.com>
phy: rockchip: naneng-combphy: add SGMII MAC selection for RK3568
This commit adds support for routing the SGMII interface to either GMAC0
or GMAC1 on RK3568 by introducing the rockchip,sgmii-mac-sel DT property.
Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 229)
> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
[ ... ]
> @@ -375,6 +377,9 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
>
> priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
>
> + priv->sgmii_mac_sel = 0;
> + device_property_read_u32(dev, "rockchip,sgmii-mac-sel", &priv->sgmii_mac_sel);
> +
> priv->phy_rst = devm_reset_control_get_exclusive(dev, "phy");
[ ... ]
> @@ -873,6 +878,8 @@ static int rk3568_combphy_cfg(struct rockchip_combphy_priv *priv)
> break;
>
> case PHY_TYPE_SGMII:
> + rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_sgmii_mac_sel,
> + priv->sgmii_mac_sel > 0);
[Severity: High]
Does this break device tree backward compatibility?
When the rockchip,sgmii-mac-sel property is missing from an older device
tree, priv->sgmii_mac_sel defaults to 0 in rockchip_combphy_parse_dt().
By unconditionally calling rockchip_combphy_param_write() here, the driver
now actively clears the MAC selection bit to 0 (GMAC0).
Because the hardware reset value is GMAC1, this actively overrides the default
hardware and bootloader state that older device trees were relying on.
Could the code check if the property was actually present before writing
to the register, to ensure the hardware default is preserved for existing
setups?
> rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_xpcs_phy_ready, true);
> rockchip_combphy_param_write(priv->phy_grf, &cfg->pipe_phymode_sel, true);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=3
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [RFC PATCH 05/10] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes
From: sashiko-bot @ 2026-07-15 19:14 UTC (permalink / raw)
To: Coia Prant
Cc: robh, devicetree, neil.armstrong, linux-phy, vkoul, olteanv,
conor+dt
In-Reply-To: <20260714191341.690906-6-coiaprant@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Devicetree anti-pattern: dummy fixed-clock nodes used for internal SoC clocks and left unassigned.
- [Low] State mismatch: fixed-clock nodes are enabled by default despite commit message claims.
--
--- Patch [5]: [RFC PATCH 05/10] arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes ---
commit b47e1fcebd43374d6431fc20ffa64a0397513d68
Author: Coia Prant <coiaprant@gmail.com>
arm64: dts: rockchip: rk3568: add XPCS and fixed-clock nodes
The RK3568 SoC integrates a Synopsys DesignWare XPCS that provides
the Physical Coding Sublayer for 1000BASE-X, SGMII, and QSGMII
interfaces via its four MII ports. Add the XPCS device node and
its pcs-mii sub-nodes to the SoC device tree.
> Also add two fixed-clock nodes (xpcs_gmac0_clk and xpcs_gmac1_clk)
> providing the 125 MHz reference clock for the GMACs when operating
> with XPCS. These clocks are used as the assigned-clock-parents
> for the respective GMAC nodes.
[Severity: Low]
The commit message states these clocks are used as assigned-clock-parents
for the respective GMAC nodes, but the patch doesn't appear to add the
assigned-clock-parents properties to the gmac0 or gmac1 nodes. Are these
assignments missing?
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> index 3bc653f027f1f..989e164c0eb39 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
[ ... ]
> + xpcs_gmac0_clk: xpcs-gmac0-clock {
> + compatible = "fixed-clock";
> + clock-frequency = <125000000>;
> + clock-output-names = "clk_gmac0_xpcs_mii";
> + #clock-cells = <0>;
> + };
[Severity: Low]
Should this internal SoC clock be modeled as a clock provider from the
generating XPCS IP node using #clock-cells, rather than using a dummy
fixed-clock?
Additionally, the commit message states "All nodes are left disabled by
default", but this node is missing the status = "disabled" property. Will
this leave the clock unconditionally enabled?
> +
> + xpcs_gmac1_clk: xpcs-gmac1-clock {
> + compatible = "fixed-clock";
> + clock-frequency = <125000000>;
> + clock-output-names = "clk_gmac1_xpcs_mii";
> + #clock-cells = <0>;
> + };
[Severity: Low]
Similarly, does this node also need a status = "disabled" property to align
with the behavior described in the commit message?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@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: [RFC PATCH 02/10] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property
From: sashiko-bot @ 2026-07-15 19:14 UTC (permalink / raw)
To: Coia Prant
Cc: olteanv, devicetree, linux-phy, vkoul, robh, conor+dt,
neil.armstrong
In-Reply-To: <20260714191341.690906-3-coiaprant@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Logical contradiction between the property's documented default value and the commit message's explicit configuration requirement.
- [Low] The `rockchip,sgmii-mac-sel` property is defined globally and not conditionally constrained to the RK3568 SoC.
--
--- Patch [2]: [RFC PATCH 02/10] dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property ---
commit 0d950b4055dd144fecf5059f16535f2c8d94b5d3
Author: Coia Prant <coiaprant@gmail.com>
dt-bindings: phy: rockchip: naneng-combphy: add rockchip,sgmii-mac-sel property
This commit adds the "rockchip,sgmii-mac-sel" property to the Rockchip
Naneng combphy device tree binding. It allows selecting between GMAC0
and GMAC1 for the SGMII interface on the RK3568 SoC.
> This is necessary for boards such as the Ariaboard Photonicat, where
> the SGMII interface is connected to GMAC0 and needs to be explicitly
> configured.
[Severity: Medium]
Is it actually necessary to explicitly configure the property to 0 for
GMAC0?
The schema defines the default as 0, which implies that omitting the property
would naturally default the hardware to GMAC0. If explicit configuration is
required, does this mean the hardware or bootloader leaves the register at 1,
making the `default: 0` documented in the schema incorrect?
> diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
> index 379b08bd9e97a..6173192e31ab2 100644
> --- a/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
> +++ b/Documentation/devicetree/bindings/phy/phy-rockchip-naneng-combphy.yaml
[ ... ]
> @@ -80,6 +80,13 @@ properties:
> description:
> Some additional pipe settings are accessed through GRF regs.
>
> + rockchip,sgmii-mac-sel:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1]
[Severity: Low]
Should this property be conditionally restricted to the RK3568 compatible
string?
Because it is defined in the top-level properties block, it will pass
validation for all other hardware variants (like RK3528 or RK3588) that do
not actually support SGMII. Could an `if-then` block in the `allOf` section
be used to set this property to false for compatibles other than
`rockchip,rk3568-naneng-combphy`?
> + default: 0
> + description:
> + Select gmac0 or gmac1 to be used as SGMII controller.
> +
> "#phy-cells":
> const: 1
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@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 v13 00/35] phy: rockchip: usbdp: Clean up the mess
From: Sebastian Reichel @ 2026-07-15 18:24 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Heiko Stuebner, Frank Wang,
Thinh Nguyen, Greg Kroah-Hartman, linux-usb, linux-phy
Cc: Andy Yan, Dmitry Baryshkov, Yubing Zhang, Alexey Charkov,
linux-phy, linux-arm-kernel, linux-rockchip, linux-kernel, kernel,
devicetree, linux-usb, Sashiko, William Wu
In-Reply-To: <20260714-rockchip-usbdp-cleanup-v13-0-6cb3e769d4c5@collabora.com>
[-- Attachment #1.1: Type: text/plain, Size: 726 bytes --]
Hi,
I've gone through the Sashiko feedback for v13 and I do not plan
to submit a v14 for Sashiko feedback at this point. The issues
it reported in v13 are either fixed in later patches - especially
all those "pre-existing problems" - or do not apply.
The series fixes a massive amount of problems in the USBDP driver
and unblocks USB-C DP AltMode at the same point. It would be good if
we get this into 7.3, so that the DRM series is unblocked for 7.4
allowing users to have USB-C DP AltMode in 2027.
My proposed merge strategy is to route everything through the PHY
subsystem as the changes in the DWC3 driver are not very complex
(purely additions), so a merge conflict should be easy to solve.
Greetings,
-- Sebastian
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 112 bytes --]
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [RFC PATCH 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver
From: Andrew Lunn @ 2026-07-15 16:18 UTC (permalink / raw)
To: Coia Prant
Cc: Maxime Chevallier, kuba, davem, edumazet, pabeni, andrew+netdev,
robh, krzk+dt, heiko, netdev, linux-rockchip, devicetree,
linux-arm-kernel, linux-phy
In-Reply-To: <CALj3r0h4cFd-tp9-UVxihL9-a1W7_4nvnKoXW_cP5WECKc66Hw@mail.gmail.com>
> This is intentional. The SerDes is attached to the XPCS node because
> on RK3568, a single SerDes serves all four XPCS MII ports in QSGMII
> mode.
This is not the only device supporting QSGMII, and so the issues you
are addressing should be common to many QSGMII implementations.
Please take a step back. What would a generic solution look like?
Can the reference counting be placed into the core somewhere? The
common clock framework allows a clock to be enabled and disabled by
multiple consumers, and the core clock code does the reference
counting, only calling into the clock driver when state change is
actually needed. Maybe the generic PHY core needs something similar?
Andrew
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [RFC PATCH 01/10] net: stmmac: move XPCS lifetime management to platform drivers
From: Andrew Lunn @ 2026-07-15 16:09 UTC (permalink / raw)
To: Coia Prant
Cc: Maxime Chevallier, kuba, davem, edumazet, pabeni, andrew+netdev,
robh, krzk+dt, heiko, netdev, linux-rockchip, devicetree,
linux-arm-kernel, linux-phy, Christian Marangi
In-Reply-To: <CALj3r0gWyfHHp0S4=71M1-TjgHKcgGax4Lr0DzPi37Q69-iCwA@mail.gmail.com>
> I actually spent all night debugging call traces caused by the current
> stmmac PCS lifetime management, and it was not a pleasant experience.
> The code feels like accumulated technical debt that should be cleaned
> up.
Russell King will strongly agree with you, if he was around. The
phylink integration into stmmac causes a lot of pain, and he wanted to
rip it all out and start again. But doing that without breaking stuff
is going to be interesting.
If you feel up to it, have a go. But take a good look around first,
make sure you understand the different variant, internal vs external
PCS.
Andrew
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v6 6/6] phy: fsl-imx8mq-usb: keep PHY power domain runtime always-on for i.MX8MP
From: Frank Li @ 2026-07-15 15:32 UTC (permalink / raw)
To: Xu Yang
Cc: Vinod Koul, Neil Armstrong, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Jun Li, linux-phy, imx,
linux-arm-kernel, linux-kernel, Xu Yang
In-Reply-To: <20260715-imx8mp-usb-phy-improvement-v6-6-00d95e270e4c@nxp.com>
On Wed, Jul 15, 2026 at 07:34:01PM +0800, Xu Yang wrote:
> From: Xu Yang <xu.yang_2@nxp.com>
>
> On i.MX8MP, the USB PHY has a dedicated power domain that was previously
> never powered off at runtime. With the introduction of runtime PM support,
> the power domain will be powered off if the device is runtime suspended,
> which breaks USB wakeup functionality.
>
> To preserve wakeup functionality, mark the PHY power domain as runtime
> always-on for i.MX8MP platform. To limit the behavior to i.MX8MP, add a
> need_genpd_rpm_on to imx8mq_usb_phy_drvdata and set it as true for i.MX8MP.
>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
>
> ---
> Changes in v6:
> - set need_genpd_rpm_on flag for i.MX8MP and call dev_pm_genpd_rpm_always_on()
> if the flag is true
> Changes in v5:
> - no changes
> Changes in v4:
> - no changes
> Changes in v3:
> - new patch
> ---
> drivers/phy/freescale/phy-fsl-imx8mq-usb.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index f0bdfa75d586..2398bda50eb8 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> @@ -9,6 +9,7 @@
> #include <linux/of.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> #include <linux/pm_runtime.h>
> #include <linux/regulator/consumer.h>
> #include <linux/regmap.h>
> @@ -134,6 +135,7 @@ struct imx8mq_usb_phy {
>
> struct imx8mq_usb_phy_drvdata {
> const struct phy_ops *ops;
> + bool need_genpd_rpm_on;
> };
>
> static void tca_blk_orientation_set(struct tca_blk *tca,
> @@ -669,6 +671,7 @@ static const struct imx8mq_usb_phy_drvdata imx8mq_usb_phy_data = {
>
> static const struct imx8mq_usb_phy_drvdata imx8mp_usb_phy_data = {
> .ops = &imx8mp_usb_phy_ops,
> + .need_genpd_rpm_on = true,
> };
>
> static const struct imx8mq_usb_phy_drvdata imx95_usb_phy_data = {
> @@ -739,6 +742,12 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
> if (!phy_data)
> return -EINVAL;
>
> + if (phy_data->need_genpd_rpm_on) {
> + ret = dev_pm_genpd_rpm_always_on(dev, true);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to set genpd rpm always on\n");
> + }
> +
need set false at driver remove?
Frank
> imx_phy->phy = devm_phy_create(dev, NULL, phy_data->ops);
> if (IS_ERR(imx_phy->phy))
> return PTR_ERR(imx_phy->phy);
>
> --
> 2.34.1
>
>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v6 5/6] phy: fsl-imx8mq-usb: introduce per-variant driver data structure
From: Frank Li @ 2026-07-15 15:29 UTC (permalink / raw)
To: Xu Yang
Cc: Vinod Koul, Neil Armstrong, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Jun Li, linux-phy, imx,
linux-arm-kernel, linux-kernel, Xu Yang
In-Reply-To: <20260715-imx8mp-usb-phy-improvement-v6-5-00d95e270e4c@nxp.com>
On Wed, Jul 15, 2026 at 07:34:00PM +0800, Xu Yang wrote:
> From: Xu Yang <xu.yang_2@nxp.com>
>
> Replace direct use of phy_ops pointer in of_device_id .data with a
> dedicated imx8mq_usb_phy_drvdata structure. This allows per-variant
> driver data to be extended in the future without changing the match
> table.
>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> ---
> Changes in v6:
> - new patch
> ---
> drivers/phy/freescale/phy-fsl-imx8mq-usb.c | 29 ++++++++++++++++++++++-------
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index 79b3958ead2f..f0bdfa75d586 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> @@ -132,6 +132,9 @@ struct imx8mq_usb_phy {
> u32 comp_dis_tune;
> };
>
> +struct imx8mq_usb_phy_drvdata {
> + const struct phy_ops *ops;
> +};
>
> static void tca_blk_orientation_set(struct tca_blk *tca,
> enum typec_orientation orientation);
> @@ -660,13 +663,25 @@ static const struct phy_ops imx8mp_usb_phy_ops = {
> .owner = THIS_MODULE,
> };
>
> +static const struct imx8mq_usb_phy_drvdata imx8mq_usb_phy_data = {
> + .ops = &imx8mq_usb_phy_ops,
> +};
> +
> +static const struct imx8mq_usb_phy_drvdata imx8mp_usb_phy_data = {
> + .ops = &imx8mp_usb_phy_ops,
> +};
> +
> +static const struct imx8mq_usb_phy_drvdata imx95_usb_phy_data = {
> + .ops = &imx8mp_usb_phy_ops,
> +};
> +
> static const struct of_device_id imx8mq_usb_phy_of_match[] = {
> {.compatible = "fsl,imx8mq-usb-phy",
> - .data = &imx8mq_usb_phy_ops,},
> + .data = &imx8mq_usb_phy_data,},
> {.compatible = "fsl,imx8mp-usb-phy",
> - .data = &imx8mp_usb_phy_ops,},
> + .data = &imx8mp_usb_phy_data,},
> {.compatible = "fsl,imx95-usb-phy",
> - .data = &imx8mp_usb_phy_ops,},
> + .data = &imx95_usb_phy_data,},
> { }
> };
> MODULE_DEVICE_TABLE(of, imx8mq_usb_phy_of_match);
> @@ -684,7 +699,7 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
> struct phy_provider *phy_provider;
> struct device *dev = &pdev->dev;
> struct imx8mq_usb_phy *imx_phy;
> - const struct phy_ops *phy_ops;
> + const struct imx8mq_usb_phy_drvdata *phy_data;
> int ret;
>
> imx_phy = devm_kzalloc(dev, sizeof(*imx_phy), GFP_KERNEL);
> @@ -720,11 +735,11 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
> if (ret)
> return dev_err_probe(dev, ret, "failed to enable runtime PM\n");
>
> - phy_ops = of_device_get_match_data(dev);
> - if (!phy_ops)
> + phy_data = of_device_get_match_data(dev);
> + if (!phy_data)
> return -EINVAL;
>
> - imx_phy->phy = devm_phy_create(dev, NULL, phy_ops);
> + imx_phy->phy = devm_phy_create(dev, NULL, phy_data->ops);
> if (IS_ERR(imx_phy->phy))
> return PTR_ERR(imx_phy->phy);
>
>
> --
> 2.34.1
>
>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v6 3/6] phy: fsl-imx8mq-usb: add runtime PM support
From: Frank Li @ 2026-07-15 15:28 UTC (permalink / raw)
To: Xu Yang
Cc: Vinod Koul, Neil Armstrong, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Jun Li, linux-phy, imx,
linux-arm-kernel, linux-kernel, Xu Yang
In-Reply-To: <20260715-imx8mp-usb-phy-improvement-v6-3-00d95e270e4c@nxp.com>
On Wed, Jul 15, 2026 at 07:33:58PM +0800, Xu Yang wrote:
> From: Xu Yang <xu.yang_2@nxp.com>
>
> Add runtime PM to ensure the PHY is properly powered and clocked during
> register access, preventing potential system hangs.
>
> It guards register access in the following scenarios:
> - PHY operations: init() and power_on/off() callbacks are guarded by
> phy core
> - Type-C orientation switching when PHY/Controller are suspended which
> needs explicitly care
> - Future PHY control port register regmap debugfs access
>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
>
> ---
> Changes in v6:
> - use devm_pm_runtime_enable() to disable runtime PM when probe fails
> - simply pm_runtime_get_sync/disable/put_noidle() to pm_runtime_resume()
> Changes in v5:
> - use non-devm PM runtime callback to correctly enable/disable clocks
> when unbind the device
> Changes in v4:
> - replace guard() with PM_RUNTIME_ACQUIRE()
> Changes in v3:
> - new patch
> ---
> drivers/phy/freescale/phy-fsl-imx8mq-usb.c | 67 +++++++++++++++++++++---------
> 1 file changed, 48 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index 3a5788c609e1..bc804d2b5aee 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> @@ -9,6 +9,7 @@
> #include <linux/of.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> #include <linux/regulator/consumer.h>
> #include <linux/usb/typec_mux.h>
>
> @@ -136,17 +137,15 @@ static int tca_blk_typec_switch_set(struct typec_switch_dev *sw,
> {
> struct imx8mq_usb_phy *imx_phy = typec_switch_get_drvdata(sw);
> struct tca_blk *tca = imx_phy->tca;
> - int ret;
>
> if (tca->orientation == orientation)
> return 0;
>
> - ret = clk_prepare_enable(imx_phy->clk);
> - if (ret)
> - return ret;
> + PM_RUNTIME_ACQUIRE(&imx_phy->phy->dev, pm);
> + if (PM_RUNTIME_ACQUIRE_ERR(&pm))
> + return -ENXIO;
>
> tca_blk_orientation_set(tca, orientation);
> - clk_disable_unprepare(imx_phy->clk);
>
> return 0;
> }
> @@ -620,16 +619,6 @@ static int imx8mq_phy_power_on(struct phy *phy)
> if (ret)
> return ret;
>
> - ret = clk_prepare_enable(imx_phy->clk);
> - if (ret)
> - return ret;
> -
> - ret = clk_prepare_enable(imx_phy->alt_clk);
> - if (ret) {
> - clk_disable_unprepare(imx_phy->clk);
> - return ret;
> - }
> -
> /* Disable rx term override */
> value = readl(imx_phy->base + PHY_CTRL6);
> value &= ~PHY_CTRL6_RXTERM_OVERRIDE_SEL;
> @@ -648,8 +637,6 @@ static int imx8mq_phy_power_off(struct phy *phy)
> value |= PHY_CTRL6_RXTERM_OVERRIDE_SEL;
> writel(value, imx_phy->base + PHY_CTRL6);
>
> - clk_disable_unprepare(imx_phy->alt_clk);
> - clk_disable_unprepare(imx_phy->clk);
> regulator_disable(imx_phy->vbus);
>
> return 0;
> @@ -686,6 +673,7 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct imx8mq_usb_phy *imx_phy;
> const struct phy_ops *phy_ops;
> + int ret;
>
> imx_phy = devm_kzalloc(dev, sizeof(*imx_phy), GFP_KERNEL);
> if (!imx_phy)
> @@ -693,13 +681,13 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, imx_phy);
>
> - imx_phy->clk = devm_clk_get(dev, "phy");
> + imx_phy->clk = devm_clk_get_enabled(dev, "phy");
> if (IS_ERR(imx_phy->clk)) {
> dev_err(dev, "failed to get imx8mq usb phy clock\n");
> return PTR_ERR(imx_phy->clk);
> }
>
> - imx_phy->alt_clk = devm_clk_get_optional(dev, "alt");
> + imx_phy->alt_clk = devm_clk_get_optional_enabled(dev, "alt");
> if (IS_ERR(imx_phy->alt_clk))
> return dev_err_probe(dev, PTR_ERR(imx_phy->alt_clk),
> "Failed to get alt clk\n");
> @@ -708,6 +696,11 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
> if (IS_ERR(imx_phy->base))
> return PTR_ERR(imx_phy->base);
>
> + pm_runtime_set_active(dev);
> + ret = devm_pm_runtime_enable(dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to enable runtime PM\n");
> +
no place to call pm runtime suspend, so whole driver's life cycle runtime
pm is on, is it expected?
Frank
> phy_ops = of_device_get_match_data(dev);
> if (!phy_ops)
> return -EINVAL;
> @@ -737,15 +730,51 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
>
> static void imx8mq_usb_phy_remove(struct platform_device *pdev)
> {
> + struct device *dev = &pdev->dev;
> +
> + /* To allow devm-managed resources to be released cleanly */
> + if (pm_runtime_suspended(dev))
> + pm_runtime_resume(dev);
> +}
> +
> +static int imx8mq_usb_phy_runtime_suspend(struct device *dev)
> +{
> + struct imx8mq_usb_phy *imx_phy = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(imx_phy->alt_clk);
> + clk_disable_unprepare(imx_phy->clk);
>
> + return 0;
> }
>
> +static int imx8mq_usb_phy_runtime_resume(struct device *dev)
> +{
> + struct imx8mq_usb_phy *imx_phy = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = clk_prepare_enable(imx_phy->clk);
> + if (ret)
> + return ret;
> +
> + ret = clk_prepare_enable(imx_phy->alt_clk);
> + if (ret) {
> + clk_disable_unprepare(imx_phy->clk);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(imx8mq_usb_phy_pm_ops, imx8mq_usb_phy_runtime_suspend,
> + imx8mq_usb_phy_runtime_resume, NULL);
> +
> static struct platform_driver imx8mq_usb_phy_driver = {
> .probe = imx8mq_usb_phy_probe,
> .remove = imx8mq_usb_phy_remove,
> .driver = {
> .name = "imx8mq-usb-phy",
> .of_match_table = imx8mq_usb_phy_of_match,
> + .pm = pm_ptr(&imx8mq_usb_phy_pm_ops),
> .suppress_bind_attrs = true,
> }
> };
>
> --
> 2.34.1
>
>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v3 2/2] phy: mediatek: Add support for MT8196 MIPI DSI PHY
From: AngeloGioacchino Del Regno @ 2026-07-15 11:57 UTC (permalink / raw)
To: Vinod Koul
Cc: chunfeng.yun, neil.armstrong, robh, krzk+dt, conor+dt,
chunkuang.hu, p.zabel, matthias.bgg, justin.yeh, linux-arm-kernel,
linux-mediatek, linux-phy, devicetree, linux-kernel, dri-devel,
kernel
In-Reply-To: <alZm25Ro_WdKigPd@vaman>
On 7/14/26 18:42, Vinod Koul wrote:
> On 13-07-26, 12:38, AngeloGioacchino Del Regno wrote:
>> Add support for the MIPI DSI PHY found in the MediaTek MT8196 SoC
>> and its variants.
>> This PHY has a different register layout and provides support for
>> more hardware features compared to the previous generation.
>>
>> This initial driver only adds support for basic functionality that
>> is necessary to drive MIPI DSI displays as a D-PHY.
>>
>> Feature additions like lane-swap, DPHY/CPHY switching, dual-port,
>> and others, may be done in the future.
>
> This seems to fail to build
>
> https://patchwork.kernel.org/project/linux-phy/patch/20260713103805.23030-2-angelogioacchino.delregno@collabora.com/
>
The failure in your log is:
make[7]: *** Deleting file 'drivers/gpu/drm/msm/msm_gem.o'
Segmentation fault (core dumped)
make[7]: *** [../scripts/Makefile.build:289: drivers/gpu/drm/msm/msm_gem.o] Error 139
make[6]: *** [../scripts/Makefile.build:549: drivers/gpu/drm/msm] Error 2
...your build server is experiencing some hardware issues maybe? :-)
In any case, I've just rechecked this series, and it builds fine here, with:
ce1b90b20bbc (HEAD) phy: mediatek: Add support for MT8196 MIPI DSI PHY
522a9d162c98 dt-bindings: phy: mediatek,dsi-phy: Add support for MT8196
49362394dad7 (tag: next-20260713, next/master, next/HEAD) Add linux-next specific
files for 20260713
With CONFIG_PHY_MTK_MIPI_DSI=m :
AR drivers/phy/mediatek/built-in.a
CC [M] drivers/phy/mediatek/phy-mtk-mipi-dsi.o
CC [M] drivers/phy/mediatek/phy-mtk-mipi-dsi-mt8173.o
CC [M] drivers/phy/mediatek/phy-mtk-mipi-dsi-mt8183.o
CC [M] drivers/phy/mediatek/phy-mtk-mipi-dsi-mt8196.o
UPD kernel/config_data
GZIP kernel/config_data.gz
CC kernel/configs.o
LD [M] drivers/phy/mediatek/phy-mtk-mipi-dsi-drv.o
AR drivers/phy/built-in.a
AR kernel/built-in.a
AR drivers/built-in.a
AR built-in.a
AR built-in-fixup.a
COPY vmlinux.a
LD vmlinux.o
MODPOST Module.symvers
CC .vmlinux.export.o
UPD include/generated/utsversion.h
CC init/version-timestamp.o
KSYMS .tmp_vmlinux0.kallsyms.S
AS .tmp_vmlinux0.kallsyms.o
LD .tmp_vmlinux1
LD [M] drivers/phy/mediatek/phy-mtk-mipi-dsi-drv.ko
NM .tmp_vmlinux1.syms
......[ etc etc ]......
With CONFIG_PHY_MTK_MIPI_DSI=y :
CC drivers/phy/mediatek/phy-mtk-mipi-dsi.o
CC drivers/phy/mediatek/phy-mtk-mipi-dsi-mt8173.o
CC drivers/phy/mediatek/phy-mtk-mipi-dsi-mt8183.o
CC drivers/phy/mediatek/phy-mtk-mipi-dsi-mt8196.o
UPD kernel/config_data
GZIP kernel/config_data.gz
CC kernel/configs.o
AR drivers/phy/mediatek/built-in.a
AR drivers/phy/built-in.a
AR kernel/built-in.a
AR drivers/built-in.a
AR built-in.a
AR built-in-fixup.a
COPY vmlinux.a
LD vmlinux.o
MODPOST Module.symvers
......[ etc etc ]......
Regards,
Angelo
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v6 3/6] phy: fsl-imx8mq-usb: add runtime PM support
From: sashiko-bot @ 2026-07-15 11:52 UTC (permalink / raw)
To: Xu Yang; +Cc: neil.armstrong, imx, linux-phy, Frank.Li, olteanv, vkoul
In-Reply-To: <20260715-imx8mp-usb-phy-improvement-v6-3-00d95e270e4c@nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] `PM_RUNTIME_ACQUIRE` falsely succeeds when runtime PM is disabled (e.g., during system sleep), leading to a system hang when disabled hardware registers are accessed.
--
--- Patch [3]: [PATCH v6 3/6] phy: fsl-imx8mq-usb: add runtime PM support ---
commit d85e441d52a7fee829fc860ee8795964a99e4555
Author: Xu Yang <xu.yang_2@nxp.com>
phy: fsl-imx8mq-usb: add runtime PM support
This commit adds runtime PM support to ensure the PHY is properly powered
and clocked during register access. It guards register accesses in PHY
operations and Type-C orientation switching.
> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index 3a5788c609e1b..bc804d2b5aeeb 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
[ ... ]
> @@ -136,17 +137,15 @@ static int tca_blk_typec_switch_set(struct typec_switch_dev *sw,
> {
> struct imx8mq_usb_phy *imx_phy = typec_switch_get_drvdata(sw);
> struct tca_blk *tca = imx_phy->tca;
> - int ret;
>
> if (tca->orientation == orientation)
> return 0;
>
> - ret = clk_prepare_enable(imx_phy->clk);
> - if (ret)
> - return ret;
> + PM_RUNTIME_ACQUIRE(&imx_phy->phy->dev, pm);
> + if (PM_RUNTIME_ACQUIRE_ERR(&pm))
> + return -ENXIO;
[Severity: High]
Does this check falsely succeed when runtime PM is disabled, such as during
system sleep?
During system suspend, pm_runtime_force_suspend() disables runtime PM
(disable_depth > 0) and turns off the PHY clocks. If a Type-C orientation
change occurs via interrupt at this time, PM_RUNTIME_ACQUIRE (which wraps
pm_runtime_get_active with RPM_TRANSPARENT) will return success despite the
device being suspended.
The driver would then proceed to tca_blk_orientation_set(), which accesses
unclocked PHY registers, potentially causing a synchronous external abort or
system hang.
Could PM_RUNTIME_ACQUIRE_IF_ENABLED() be used here instead to safely check
if the device is active and fail appropriately if runtime PM is disabled?
>
> tca_blk_orientation_set(tca, orientation);
> - clk_disable_unprepare(imx_phy->clk);
>
> return 0;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-imx8mp-usb-phy-improvement-v6-0-00d95e270e4c@nxp.com?part=3
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v6 1/6] phy: fsl-imx8mq-usb: fix typec switch leak on probe error path
From: sashiko-bot @ 2026-07-15 11:46 UTC (permalink / raw)
To: Xu Yang; +Cc: olteanv, linux-phy, imx, vkoul, Frank.Li, neil.armstrong
In-Reply-To: <20260715-imx8mp-usb-phy-improvement-v6-1-00d95e270e4c@nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] The Type-C switch callback `tca_blk_typec_switch_set` can be invoked after the PHY driver is unbound, leading to a Use-After-Free of the driver context.
--
--- Patch [1]: [PATCH v6 1/6] phy: fsl-imx8mq-usb: fix typec switch leak on probe error path ---
commit 5e0093cfdd6e9dd648ed3549eb93bacb54164be2
Author: Felix Gu <ustc.gu@gmail.com>
phy: fsl-imx8mq-usb: fix typec switch leak on probe error path
This commit fixes a typec switch resource leak when probe fails after
imx95_usb_phy_get_tca() succeeds. It transitions the cleanup of the switch
to use devm_add_action_or_reset() so that it is properly freed on both
probe failure and driver removal.
> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index b05d80e849a14..9a33c06d6fc39 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> @@ -173,9 +173,9 @@ static struct typec_switch_dev *tca_blk_get_typec_switch(struct platform_device
> return sw;
> }
>
> -static void tca_blk_put_typec_switch(struct typec_switch_dev *sw)
> +static void tca_blk_put_typec_switch(void *data)
> {
> - typec_switch_unregister(sw);
> + typec_switch_unregister(data);
[Severity: High]
This is a pre-existing issue, but does typec_switch_unregister() guarantee
that no consumers will invoke the .set callback afterwards?
If a consumer acquires the switch using fwnode_typec_switch_get() and holds
a reference, it appears typec_switch_unregister() removes the device from
sysfs but doesn't revoke the consumer's ability to call sw_dev->set().
Because imx_phy and tca are allocated with devm_kzalloc() during probe, they
are automatically freed when the provider unbinds. Could this lead to a
use-after-free if the consumer asynchronously calls typec_switch_set():
tca_blk_typec_switch_set()
struct imx8mq_usb_phy *imx_phy = typec_switch_get_drvdata(sw);
struct tca_blk *tca = imx_phy->tca;
where imx_phy has already been freed by the devm teardown?
> }
>
> static void tca_blk_orientation_set(struct tca_blk *tca,
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-imx8mp-usb-phy-improvement-v6-0-00d95e270e4c@nxp.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 v6 6/6] phy: fsl-imx8mq-usb: keep PHY power domain runtime always-on for i.MX8MP
From: sashiko-bot @ 2026-07-15 11:44 UTC (permalink / raw)
To: Xu Yang; +Cc: olteanv, vkoul, Frank.Li, linux-phy, imx, neil.armstrong
In-Reply-To: <20260715-imx8mp-usb-phy-improvement-v6-6-00d95e270e4c@nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver unconditionally fails probe if dev_pm_genpd_rpm_always_on() returns an error. This breaks backward compatibility on configurations without generic power domains or when the device tree lacks a power-domains property.
--
--- Patch [6]: [PATCH v6 6/6] phy: fsl-imx8mq-usb: keep PHY power domain runtime always-on for i.MX8MP ---
commit 14514f8e33060037a734acad2199e7febc56def8
Author: Xu Yang <xu.yang_2@nxp.com>
Subject: phy: fsl-imx8mq-usb: keep PHY power domain runtime always-on for i.MX8MP
This commit marks the PHY power domain as runtime always-on for the i.MX8MP
platform to prevent it from being powered off during runtime suspend. This is
intended to preserve USB wakeup functionality on i.MX8MP.
> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
[ ... ]
> @@ -739,6 +742,12 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
> if (!phy_data)
> return -EINVAL;
>
> + if (phy_data->need_genpd_rpm_on) {
> + ret = dev_pm_genpd_rpm_always_on(dev, true);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to set genpd rpm always on\n");
[Severity: High]
Does this code cause probe to fail unnecessarily when generic power domains
are disabled or not configured in the device tree?
If the kernel is compiled without CONFIG_PM_GENERIC_DOMAINS,
dev_pm_genpd_rpm_always_on() will return -EOPNOTSUPP. Similarly, if the
device tree lacks a 'power-domains' property for the PHY node, it may return
-ENODEV.
Since the power domain won't be runtime-suspended by the generic power domain
framework in these cases, the wakeup issue this patch intends to fix doesn't
exist.
Should this check ignore non-fatal errors like -EOPNOTSUPP and -ENODEV to
preserve backward compatibility with older device trees and minimal kernel
configurations?
> + }
> +
> imx_phy->phy = devm_phy_create(dev, NULL, phy_data->ops);
> if (IS_ERR(imx_phy->phy))
> return PTR_ERR(imx_phy->phy);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-imx8mp-usb-phy-improvement-v6-0-00d95e270e4c@nxp.com?part=6
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH v6 6/6] phy: fsl-imx8mq-usb: keep PHY power domain runtime always-on for i.MX8MP
From: Xu Yang @ 2026-07-15 11:34 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Jun Li
Cc: linux-phy, imx, linux-arm-kernel, linux-kernel, Xu Yang
In-Reply-To: <20260715-imx8mp-usb-phy-improvement-v6-0-00d95e270e4c@nxp.com>
From: Xu Yang <xu.yang_2@nxp.com>
On i.MX8MP, the USB PHY has a dedicated power domain that was previously
never powered off at runtime. With the introduction of runtime PM support,
the power domain will be powered off if the device is runtime suspended,
which breaks USB wakeup functionality.
To preserve wakeup functionality, mark the PHY power domain as runtime
always-on for i.MX8MP platform. To limit the behavior to i.MX8MP, add a
need_genpd_rpm_on to imx8mq_usb_phy_drvdata and set it as true for i.MX8MP.
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
Changes in v6:
- set need_genpd_rpm_on flag for i.MX8MP and call dev_pm_genpd_rpm_always_on()
if the flag is true
Changes in v5:
- no changes
Changes in v4:
- no changes
Changes in v3:
- new patch
---
drivers/phy/freescale/phy-fsl-imx8mq-usb.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
index f0bdfa75d586..2398bda50eb8 100644
--- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
+++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
@@ -9,6 +9,7 @@
#include <linux/of.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
#include <linux/pm_runtime.h>
#include <linux/regulator/consumer.h>
#include <linux/regmap.h>
@@ -134,6 +135,7 @@ struct imx8mq_usb_phy {
struct imx8mq_usb_phy_drvdata {
const struct phy_ops *ops;
+ bool need_genpd_rpm_on;
};
static void tca_blk_orientation_set(struct tca_blk *tca,
@@ -669,6 +671,7 @@ static const struct imx8mq_usb_phy_drvdata imx8mq_usb_phy_data = {
static const struct imx8mq_usb_phy_drvdata imx8mp_usb_phy_data = {
.ops = &imx8mp_usb_phy_ops,
+ .need_genpd_rpm_on = true,
};
static const struct imx8mq_usb_phy_drvdata imx95_usb_phy_data = {
@@ -739,6 +742,12 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
if (!phy_data)
return -EINVAL;
+ if (phy_data->need_genpd_rpm_on) {
+ ret = dev_pm_genpd_rpm_always_on(dev, true);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to set genpd rpm always on\n");
+ }
+
imx_phy->phy = devm_phy_create(dev, NULL, phy_data->ops);
if (IS_ERR(imx_phy->phy))
return PTR_ERR(imx_phy->phy);
--
2.34.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v6 5/6] phy: fsl-imx8mq-usb: introduce per-variant driver data structure
From: Xu Yang @ 2026-07-15 11:34 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Jun Li
Cc: linux-phy, imx, linux-arm-kernel, linux-kernel, Xu Yang
In-Reply-To: <20260715-imx8mp-usb-phy-improvement-v6-0-00d95e270e4c@nxp.com>
From: Xu Yang <xu.yang_2@nxp.com>
Replace direct use of phy_ops pointer in of_device_id .data with a
dedicated imx8mq_usb_phy_drvdata structure. This allows per-variant
driver data to be extended in the future without changing the match
table.
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
Changes in v6:
- new patch
---
drivers/phy/freescale/phy-fsl-imx8mq-usb.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
index 79b3958ead2f..f0bdfa75d586 100644
--- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
+++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
@@ -132,6 +132,9 @@ struct imx8mq_usb_phy {
u32 comp_dis_tune;
};
+struct imx8mq_usb_phy_drvdata {
+ const struct phy_ops *ops;
+};
static void tca_blk_orientation_set(struct tca_blk *tca,
enum typec_orientation orientation);
@@ -660,13 +663,25 @@ static const struct phy_ops imx8mp_usb_phy_ops = {
.owner = THIS_MODULE,
};
+static const struct imx8mq_usb_phy_drvdata imx8mq_usb_phy_data = {
+ .ops = &imx8mq_usb_phy_ops,
+};
+
+static const struct imx8mq_usb_phy_drvdata imx8mp_usb_phy_data = {
+ .ops = &imx8mp_usb_phy_ops,
+};
+
+static const struct imx8mq_usb_phy_drvdata imx95_usb_phy_data = {
+ .ops = &imx8mp_usb_phy_ops,
+};
+
static const struct of_device_id imx8mq_usb_phy_of_match[] = {
{.compatible = "fsl,imx8mq-usb-phy",
- .data = &imx8mq_usb_phy_ops,},
+ .data = &imx8mq_usb_phy_data,},
{.compatible = "fsl,imx8mp-usb-phy",
- .data = &imx8mp_usb_phy_ops,},
+ .data = &imx8mp_usb_phy_data,},
{.compatible = "fsl,imx95-usb-phy",
- .data = &imx8mp_usb_phy_ops,},
+ .data = &imx95_usb_phy_data,},
{ }
};
MODULE_DEVICE_TABLE(of, imx8mq_usb_phy_of_match);
@@ -684,7 +699,7 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
struct phy_provider *phy_provider;
struct device *dev = &pdev->dev;
struct imx8mq_usb_phy *imx_phy;
- const struct phy_ops *phy_ops;
+ const struct imx8mq_usb_phy_drvdata *phy_data;
int ret;
imx_phy = devm_kzalloc(dev, sizeof(*imx_phy), GFP_KERNEL);
@@ -720,11 +735,11 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
if (ret)
return dev_err_probe(dev, ret, "failed to enable runtime PM\n");
- phy_ops = of_device_get_match_data(dev);
- if (!phy_ops)
+ phy_data = of_device_get_match_data(dev);
+ if (!phy_data)
return -EINVAL;
- imx_phy->phy = devm_phy_create(dev, NULL, phy_ops);
+ imx_phy->phy = devm_phy_create(dev, NULL, phy_data->ops);
if (IS_ERR(imx_phy->phy))
return PTR_ERR(imx_phy->phy);
--
2.34.1
--
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