* [PATCH 0/2] PHY: Add MediaTek PCI-Express Gen4 S-PHY Driver
From: AngeloGioacchino Del Regno @ 2026-07-16 15:13 UTC (permalink / raw)
To: chunfeng.yun
Cc: vkoul, neil.armstrong, robh, krzk+dt, conor+dt, matthias.bgg,
angelogioacchino.delregno, linux-arm-kernel, linux-mediatek,
linux-phy, devicetree, linux-kernel, kernel
This adds a driver for the PCI-Express Gen4 "S-PHY" found in the
Genio MT8894, Kompanio MT8196, Dimensity MT6991 SoCs (which are
all variants of the same chip).
This was successfully tested on MT8894 and MT8196.
AngeloGioacchino Del Regno (2):
dt-bindings: phy: Document MT8196 MediaTek PCI-Express Gen4 S-PHY
phy: mediatek: Add support for PCI-Express Gen4 S-PHY
.../phy/mediatek,mt8196-pcie-sphy.yaml | 91 +++
drivers/phy/mediatek/Kconfig | 9 +
drivers/phy/mediatek/Makefile | 1 +
drivers/phy/mediatek/phy-mtk-pcie-sphy.c | 560 ++++++++++++++++++
4 files changed, 661 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/mediatek,mt8196-pcie-sphy.yaml
create mode 100644 drivers/phy/mediatek/phy-mtk-pcie-sphy.c
--
2.54.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v9 1/2] dt-bindings: phy: qcom: Add CSI2 C-PHY/DPHY schema
From: Bryan O'Donoghue @ 2026-07-16 12:52 UTC (permalink / raw)
To: Loic Poulain
Cc: Bryan O'Donoghue, Vladimir Zapolskiy, Vinod Koul,
Kishon Vijay Abraham I, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, linux-arm-msm, linux-phy,
linux-media, devicetree, linux-kernel
In-Reply-To: <CAFEp6-3U66hvi0iZrtRbyY3B-uwEap6xDFHFkVoaEBwWhV74Uw@mail.gmail.com>
On 16/07/2026 12:28, Loic Poulain wrote:
>> I just wonder if there's any real upside to all that additional work and
>> delay/churn.
> There is no really need to support runtime configuration as of now
> since we only support D-PHY, so the delta for v10*should* be tiny,
> either a bus-type sanity read in parse_routing(), or even nothing at
> DT parse and a future configure() branch when C-PHY lands.
Right, well I tend to agree that a runtime configure() is out of scope,
so, I'm going to stick to Rob's guidance.
If you look at other upstream stuff you will see phy-cells = 0 for
single mode PHYs and phy-cells = 1 for PHYs that can be in any mode.
Consumer configures the mode in DT like every other upstream MIPI CSI2 PHY.
---
bod
--
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-16 12:41 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: <omitpksxmr657vlmjs3ybzhiq6zj4znrsp2tereofqees27z36@m3lhmrbufqay>
On Thu, Jul 16, 2026 at 07:32:59PM +0800, Xu Yang wrote:
> On Wed, Jul 15, 2026 at 10:28:05AM -0500, Frank Li wrote:
> > 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?
>
> First, at the end of __driver_probe_device(), it will call pm_request_idle()
> for the device. So runtime_suspend() will run.
> Second, this platform device is the parent of created phy device, the runtime status
> of phy device is managed by phy core, so this platform device will be managed by phy
> core indirectly.
>
> Just found it's not suitable to call devm_pm_runtime_enable() here. Because
>
> pm_runtime_disable_action()
> pm_runtime_dont_use_autosuspend()
> update_autosuspend()
> rpm_idle()
>
> then after remove(), runtime suspend func will still be called. Clocks may be
> disabled twice. I will use non-devm version pm_runtime_enable() in next version.
This should be common problem. Your case should be typical one.
try devm_pm_runtime_set_active_enabled().
Frank
>
> Thanks,
> Xu Yang
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v4 1/8] phy: rockchip: samsung-hdptx: Fix rate recalculation for high bpc
From: Andy Yan @ 2026-07-16 12:27 UTC (permalink / raw)
To: Cristian Ciocaltea
Cc: Vinod Koul, Neil Armstrong, Heiko Stuebner, Algea Cao,
Dmitry Baryshkov, kernel, linux-phy, linux-arm-kernel,
linux-rockchip, linux-kernel, Thomas Niederprüm,
Simon Wright
In-Reply-To: <20260612-hdptx-clk-fixes-v4-1-ce5e1d456cda@collabora.com>
Hello，
Cristian Ciocaltea <cristian.ciocaltea@collabora.com> 于2026年6月12日周五 07:45写道：
>
> The PHY PLL can be programmed by an external component, e.g. the
> bootloader, just before the recalc_rate() callback is invoked during
> devm_clk_hw_register() in the probe path.
>
> Therefore rk_hdptx_phy_clk_recalc_rate() finds the PLL enabled and
> attempts to compute the clock rate, while making use of the bpc value
> from the HDMI PHY configuration, which always defaults to 8 because
> phy_configure() was not run at that point. As a consequence, the
> (re)calculated rate is incorrect when the actual bpc was higher than 8.
>
> Do not rely on any of the hdmi_cfg members when computing the clock rate
> and, instead, read the required input data (i.e. bpc), directly from the
> hardware registers.
>
> Fixes: 3481fc04d969 ("phy: rockchip: samsung-hdptx: Compute clk rate from PLL config")
> Tested-by: Thomas Niederprüm <dubito@online.de>
> Tested-by: Simon Wright <simon@symple.nz>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
Reviewed-by：Andy Yan <andyshrk@gmail.com>
Thanks
> ---
> drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> index 2d973bc37f07..710603afff86 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> @@ -2168,7 +2168,7 @@ static u64 rk_hdptx_phy_clk_calc_rate_from_pll_cfg(struct rk_hdptx_phy *hdptx)
> struct lcpll_config lcpll_hw;
> struct ropll_config ropll_hw;
> u64 fout, sdm;
> - u32 mode, val;
> + u32 mode, bpc, val;
> int ret, i;
>
> ret = regmap_read(hdptx->regmap, CMN_REG(0008), &mode);
> @@ -2266,6 +2266,7 @@ static u64 rk_hdptx_phy_clk_calc_rate_from_pll_cfg(struct rk_hdptx_phy *hdptx)
> if (ret)
> return 0;
> ropll_hw.pms_sdiv = ((val & PLL_PCG_POSTDIV_SEL_MASK) >> 4) + 1;
> + bpc = (FIELD_GET(PLL_PCG_CLK_SEL_MASK, val) << 1) + 8;
>
> fout = PLL_REF_CLK * ropll_hw.pms_mdiv;
> if (ropll_hw.sdm_en) {
> @@ -2280,7 +2281,7 @@ static u64 rk_hdptx_phy_clk_calc_rate_from_pll_cfg(struct rk_hdptx_phy *hdptx)
> fout = fout + sdm;
> }
>
> - return div_u64(fout * 2, ropll_hw.pms_sdiv * 10);
> + return DIV_ROUND_CLOSEST_ULL(fout * 2 * 8, ropll_hw.pms_sdiv * 10 * bpc);
> }
>
> static unsigned long rk_hdptx_phy_clk_recalc_rate(struct clk_hw *hw,
> @@ -2288,19 +2289,13 @@ static unsigned long rk_hdptx_phy_clk_recalc_rate(struct clk_hw *hw,
> {
> struct rk_hdptx_phy *hdptx = to_rk_hdptx_phy(hw);
> u32 status;
> - u64 rate;
> int ret;
>
> ret = regmap_read(hdptx->grf, GRF_HDPTX_CON0, &status);
> if (ret || !(status & HDPTX_I_PLL_EN))
> return 0;
>
> - rate = rk_hdptx_phy_clk_calc_rate_from_pll_cfg(hdptx);
> -
> - if (hdptx->hdmi_cfg.mode == PHY_HDMI_MODE_FRL)
> - return rate;
> -
> - return DIV_ROUND_CLOSEST_ULL(rate * 8, hdptx->hdmi_cfg.bpc);
> + return rk_hdptx_phy_clk_calc_rate_from_pll_cfg(hdptx);
> }
>
> static int rk_hdptx_phy_clk_determine_rate(struct clk_hw *hw,
>
> --
> 2.54.0
>
>
--
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: Xu Yang @ 2026-07-16 11:32 UTC (permalink / raw)
To: Frank Li
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: <alenBaMOTSPo7DJV@SMW015318>
On Wed, Jul 15, 2026 at 10:28:05AM -0500, Frank Li wrote:
> 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?
First, at the end of __driver_probe_device(), it will call pm_request_idle()
for the device. So runtime_suspend() will run.
Second, this platform device is the parent of created phy device, the runtime status
of phy device is managed by phy core, so this platform device will be managed by phy
core indirectly.
Just found it's not suitable to call devm_pm_runtime_enable() here. Because
pm_runtime_disable_action()
pm_runtime_dont_use_autosuspend()
update_autosuspend()
rpm_idle()
then after remove(), runtime suspend func will still be called. Clocks may be
disabled twice. I will use non-devm version pm_runtime_enable() in next version.
Thanks,
Xu Yang
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v9 1/2] dt-bindings: phy: qcom: Add CSI2 C-PHY/DPHY schema
From: Loic Poulain @ 2026-07-16 11:28 UTC (permalink / raw)
To: Bryan O'Donoghue
Cc: Bryan O'Donoghue, Vladimir Zapolskiy, Vinod Koul,
Kishon Vijay Abraham I, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, linux-arm-msm, linux-phy,
linux-media, devicetree, linux-kernel
In-Reply-To: <c731b03e-b550-48af-b338-37ac53018247@linaro.org>
Hi Bryan,
On Thu, Jul 16, 2026 at 11:42 AM Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 16/07/2026 09:56, Loic Poulain wrote:
> > On Mon, Jul 13, 2026 at 1:18 AM Bryan O'Donoghue <bod@kernel.org> wrote:
> >>
> >> On 09/07/2026 08:40, Vladimir Zapolskiy wrote:
> >>>> As explained in the cover letter: I'm following guidance from Rob on that.
> >>> Pehaps you've misread a given review comment, because Rob's guidance is
> >>> to remove 'phy-type' property,
> >>
> >> I'll just quote verbatim "Perhaps putting this in phy cells would be
> >> better because the consumer decides on the mode."
> >>
> >> That is not "remove phy-type" - it's "move the mode into the cell". I'm
> >> shifting this back to the DT schema maintainers since, I take their
> >> feedback literally, seriously and canonically.
> >>
> >> Any of the triumvirate will do, Rob, Krzysztof or Conor.
> >>
> >> Absent a different answer from the DT schema guys, v10 ships with the
> >> consumer deciding the mode as per Rob's original guidance.
> >
> > On the #phy-cells question, IMO we should drop #phy-cells to 0 and
> > derive the C-PHY/D-PHY mode from the endpoint bus-type. The operating
> > mode is really a configuration of the link rather than a hardware
> > description/limitation of the PHY block itself,and the sensor endpoint
> > is conventionally where the bus type originates. I guess this costs us
> > nothing on the driver side today, we only support D-PHY, so the
> > mode-check just moves into the endpoint parsing we already do in
> > phy_qcom_mipi_csi2_parse_routing(). It can be a minimal
> > fwnode_property_read_u32(ep, "bus-type", ...) sanity check and adding
> > C-PHY later is then a one-line case MEDIA_BUS_TYPE_CSI2_CPHY
> > with no DT ABI change.
>
> So I take your point on link-type - in theory you can select the mode
> from the sensor @ config time. The established practice is that the
> consumer decides the PHY mode, so if we wanted to make this a _runtime_
> configuration - then that should be information that is passed to the
> PHY via configure() not via DT because we've said its a link-mode
> abstracted from the DT entirely.
>
> In the extreme case I have a user-space utility selecting sensor mode,
> which the CSI controller parses and then signals the PHY to switch modes.
>
> If its a run-time config, then its a run-time config and DT should be
> excluded from the mode selection tree.
>
> I'm in fact willing to entertain this idea - but it will require passing
> the PHY mode to configure() not consuming - DT data from the endpoint.
>
> My core point here is both acknowleding Rob's point the consumer decides
> the mode - and _if_ we go the runtime configuration route, then actually
> making the selection @ configure not at probe/dt parse time.
I agree this is/could-be a runtime configuration/negotiation.
> I just wonder if there's any real upside to all that additional work and
> delay/churn.
There is no really need to support runtime configuration as of now
since we only support D-PHY, so the delta for v10 *should* be tiny,
either a bus-type sanity read in parse_routing(), or even nothing at
DT parse and a future configure() branch when C-PHY lands.
Regards,
Loic
--
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: Xu Yang @ 2026-07-16 10:42 UTC (permalink / raw)
To: Frank Li
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: <aleoGO5hBWCl6ZgE@SMW015318>
On Wed, Jul 15, 2026 at 10:32:40AM -0500, Frank Li wrote:
> 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?
Not need. Because this device will be deteched from the power domain after driver removal.
Thanks,
Xu Yang
--
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: Xu Yang @ 2026-07-16 10:40 UTC (permalink / raw)
To: sashiko-reviews; +Cc: olteanv, linux-phy, imx, vkoul, Frank.Li, neil.armstrong
In-Reply-To: <20260715114626.651A81F000E9@smtp.kernel.org>
On Wed, Jul 15, 2026 at 11:46:25AM +0000, sashiko-bot@kernel.org wrote:
> 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.
It's a common issue and it should be fixed in Typec-C subsystem.
Thanks,
Xu Yang
> --
>
> --- 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 v9 1/2] dt-bindings: phy: qcom: Add CSI2 C-PHY/DPHY schema
From: Bryan O'Donoghue @ 2026-07-16 9:42 UTC (permalink / raw)
To: Loic Poulain, Bryan O'Donoghue
Cc: Vladimir Zapolskiy, Vinod Koul, Kishon Vijay Abraham I,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
linux-arm-msm, linux-phy, linux-media, devicetree, linux-kernel
In-Reply-To: <CAFEp6-2J0WGF6g0K7s--PxciO+d9MR-SCVnrWJ3u=sgQNwA59A@mail.gmail.com>
On 16/07/2026 09:56, Loic Poulain wrote:
> On Mon, Jul 13, 2026 at 1:18 AM Bryan O'Donoghue <bod@kernel.org> wrote:
>>
>> On 09/07/2026 08:40, Vladimir Zapolskiy wrote:
>>>> As explained in the cover letter: I'm following guidance from Rob on that.
>>> Pehaps you've misread a given review comment, because Rob's guidance is
>>> to remove 'phy-type' property,
>>
>> I'll just quote verbatim "Perhaps putting this in phy cells would be
>> better because the consumer decides on the mode."
>>
>> That is not "remove phy-type" - it's "move the mode into the cell". I'm
>> shifting this back to the DT schema maintainers since, I take their
>> feedback literally, seriously and canonically.
>>
>> Any of the triumvirate will do, Rob, Krzysztof or Conor.
>>
>> Absent a different answer from the DT schema guys, v10 ships with the
>> consumer deciding the mode as per Rob's original guidance.
>
> On the #phy-cells question, IMO we should drop #phy-cells to 0 and
> derive the C-PHY/D-PHY mode from the endpoint bus-type. The operating
> mode is really a configuration of the link rather than a hardware
> description/limitation of the PHY block itself,and the sensor endpoint
> is conventionally where the bus type originates. I guess this costs us
> nothing on the driver side today, we only support D-PHY, so the
> mode-check just moves into the endpoint parsing we already do in
> phy_qcom_mipi_csi2_parse_routing(). It can be a minimal
> fwnode_property_read_u32(ep, "bus-type", ...) sanity check and adding
> C-PHY later is then a one-line case MEDIA_BUS_TYPE_CSI2_CPHY
> with no DT ABI change.
So I take your point on link-type - in theory you can select the mode
from the sensor @ config time. The established practice is that the
consumer decides the PHY mode, so if we wanted to make this a _runtime_
configuration - then that should be information that is passed to the
PHY via configure() not via DT because we've said its a link-mode
abstracted from the DT entirely.
In the extreme case I have a user-space utility selecting sensor mode,
which the CSI controller parses and then signals the PHY to switch modes.
If its a run-time config, then its a run-time config and DT should be
excluded from the mode selection tree.
I'm in fact willing to entertain this idea - but it will require passing
the PHY mode to configure() not consuming - DT data from the endpoint.
My core point here is both acknowleding Rob's point the consumer decides
the mode - and _if_ we go the runtime configuration route, then actually
making the selection @ configure not at probe/dt parse time.
I just wonder if there's any real upside to all that additional work and
delay/churn.
> On data-lanes numbering, I don't have a strong opinion here. Keeping
> the current 0-based values is defensible, in this driver the value is
> used as a physical lane index and it selects the per-lane register
> bank (0x200 * n), so 0 maps to physical LN0. In that sense 0-based is
> a faithful representation of the physical lane position on the SoC.
>
> Regards,
> Loic
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v9 1/2] dt-bindings: phy: qcom: Add CSI2 C-PHY/DPHY schema
From: Loic Poulain @ 2026-07-16 8:56 UTC (permalink / raw)
To: Bryan O'Donoghue
Cc: Vladimir Zapolskiy, Bryan O'Donoghue, Vinod Koul,
Kishon Vijay Abraham I, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Neil Armstrong, linux-arm-msm, linux-phy,
linux-media, devicetree, linux-kernel
In-Reply-To: <4d7a0e59-7c34-4557-a475-8cd7fd2ad2d9@kernel.org>
On Mon, Jul 13, 2026 at 1:18 AM Bryan O'Donoghue <bod@kernel.org> wrote:
>
> On 09/07/2026 08:40, Vladimir Zapolskiy wrote:
> >> As explained in the cover letter: I'm following guidance from Rob on that.
> > Pehaps you've misread a given review comment, because Rob's guidance is
> > to remove 'phy-type' property,
>
> I'll just quote verbatim "Perhaps putting this in phy cells would be
> better because the consumer decides on the mode."
>
> That is not "remove phy-type" - it's "move the mode into the cell". I'm
> shifting this back to the DT schema maintainers since, I take their
> feedback literally, seriously and canonically.
>
> Any of the triumvirate will do, Rob, Krzysztof or Conor.
>
> Absent a different answer from the DT schema guys, v10 ships with the
> consumer deciding the mode as per Rob's original guidance.
On the #phy-cells question, IMO we should drop #phy-cells to 0 and
derive the C-PHY/D-PHY mode from the endpoint bus-type. The operating
mode is really a configuration of the link rather than a hardware
description/limitation of the PHY block itself,and the sensor endpoint
is conventionally where the bus type originates. I guess this costs us
nothing on the driver side today, we only support D-PHY, so the
mode-check just moves into the endpoint parsing we already do in
phy_qcom_mipi_csi2_parse_routing(). It can be a minimal
fwnode_property_read_u32(ep, "bus-type", ...) sanity check and adding
C-PHY later is then a one-line case MEDIA_BUS_TYPE_CSI2_CPHY
with no DT ABI change.
On data-lanes numbering, I don't have a strong opinion here. Keeping
the current 0-based values is defensible, in this driver the value is
used as a physical lane index and it selects the per-lane register
bank (0x200 * n), so 0 maps to physical LN0. In that sense 0-based is
a faithful representation of the physical lane position on the SoC.
Regards,
Loic
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v9 00/19] Add Freescale i.MX8qxp Display Controller support
From: Liu Ying @ 2026-07-16 6:49 UTC (permalink / raw)
To: Francesco Dolcini
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
linux-phy, p.zabel, airlied, simona, maarten.lankhorst, mripard,
tzimmermann, robh, krzk+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, tglx, vkoul, kishon, Frank.Li, lumag, aisheng.dong, agx,
u.kleine-koenig, marek.vasut
In-Reply-To: <20260716062612.GC9973@francesco-nb>
On Thu, Jul 16, 2026 at 08:26:12AM +0200, Francesco Dolcini wrote:
> On Thu, Jul 16, 2026 at 02:21:31PM +0800, Liu Ying wrote:
> > On Tue, Jul 14, 2026 at 09:36:27PM +0200, Francesco Dolcini wrote:
> > > On Mon, Apr 14, 2025 at 11:50:09AM +0800, Liu Ying wrote:
> > > > Hi,
> > > >
> > > > This patch series aims to add Freescale i.MX8qxp Display Controller support.
> > > >
> > > > The controller is comprised of three main components that include a blit
> > > > engine for 2D graphics accelerations, display controller for display output
> > > > processing, as well as a command sequencer.
> > >
> > > ...
> > >
> > > >
> > > > To follow up i.MX8qxp TRM, I changed the controller name to "Display Controller"
> > > > instead of the previous "DPU". "DPU" is only mentioned in the SoC block
> > > > diagram and represents the whole display subsystem which includes the display
> > > > controller and prefech engines, etc.
> > > >
> > > > With an additional patch[1] for simple-pm-bus.c, this series facilitates
> > > > testing a LVDS panel on i.MX8qxp MEK.
> > > >
> > > > Please do NOT merge patch 14-19. They are only used to facilitate testing
> > > > the LVDS panel.
> > >
> > > What's the plan to conclude this work? What's the latest status?
> > > I am looking forward to have a way to use the i.MX8QXP display with
> > > mainline, but to my understanding some required changes on the SOC dtsi
> > > are not merged.
> > >
> > > Can you help?
> >
> > I planned to add display controller DT node after prefetch engine support [1]
> > is picked up so that the prefetch engine could be enabled together with the
> > display controller. Just want to avoid a DT that only enables the display
> > controller or bypasses the prefetch engine. It makes the display controller
> > driver simpler.
> >
> > [1] https://lore.kernel.org/all/20251027-imx8-dc-prefetch-v5-0-4ecb6c6d4941@nxp.com/
>
> That series is from October 2025, is there anything blocking to move it
> forward?
It lacks a Reviewed-by tag(at least a Acked-by tag) from a drm-misc committer
for every driver patch in that series. Also, it seems that Marek(Cc'ed) is
not a big fan of [1](See i.MX95 display support discussion[2]).
[2] https://lore.kernel.org/dri-devel/aiZzxhljfyYQ68Gl@raspi/
>
> Francesco
>
>
> --
> linux-phy mailing list
> linux-phy@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/linux-phy
--
Regards,
Liu Ying
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v9 00/19] Add Freescale i.MX8qxp Display Controller support
From: Francesco Dolcini @ 2026-07-16 6:26 UTC (permalink / raw)
To: Liu Ying
Cc: Francesco Dolcini, dri-devel, devicetree, imx, linux-arm-kernel,
linux-kernel, linux-phy, p.zabel, airlied, simona,
maarten.lankhorst, mripard, tzimmermann, robh, krzk+dt, conor+dt,
shawnguo, s.hauer, kernel, festevam, tglx, vkoul, kishon,
Frank.Li, lumag, aisheng.dong, agx, u.kleine-koenig,
dmitry.baryshkov
In-Reply-To: <alh4a66fEro05ShA@raspi>
On Thu, Jul 16, 2026 at 02:21:31PM +0800, Liu Ying wrote:
> On Tue, Jul 14, 2026 at 09:36:27PM +0200, Francesco Dolcini wrote:
> > On Mon, Apr 14, 2025 at 11:50:09AM +0800, Liu Ying wrote:
> > > Hi,
> > >
> > > This patch series aims to add Freescale i.MX8qxp Display Controller support.
> > >
> > > The controller is comprised of three main components that include a blit
> > > engine for 2D graphics accelerations, display controller for display output
> > > processing, as well as a command sequencer.
> >
> > ...
> >
> > >
> > > To follow up i.MX8qxp TRM, I changed the controller name to "Display Controller"
> > > instead of the previous "DPU". "DPU" is only mentioned in the SoC block
> > > diagram and represents the whole display subsystem which includes the display
> > > controller and prefech engines, etc.
> > >
> > > With an additional patch[1] for simple-pm-bus.c, this series facilitates
> > > testing a LVDS panel on i.MX8qxp MEK.
> > >
> > > Please do NOT merge patch 14-19. They are only used to facilitate testing
> > > the LVDS panel.
> >
> > What's the plan to conclude this work? What's the latest status?
> > I am looking forward to have a way to use the i.MX8QXP display with
> > mainline, but to my understanding some required changes on the SOC dtsi
> > are not merged.
> >
> > Can you help?
>
> I planned to add display controller DT node after prefetch engine support [1]
> is picked up so that the prefetch engine could be enabled together with the
> display controller. Just want to avoid a DT that only enables the display
> controller or bypasses the prefetch engine. It makes the display controller
> driver simpler.
>
> [1] https://lore.kernel.org/all/20251027-imx8-dc-prefetch-v5-0-4ecb6c6d4941@nxp.com/
That series is from October 2025, is there anything blocking to move it
forward?
Francesco
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v9 00/19] Add Freescale i.MX8qxp Display Controller support
From: Liu Ying @ 2026-07-16 6:21 UTC (permalink / raw)
To: Francesco Dolcini
Cc: dri-devel, devicetree, imx, linux-arm-kernel, linux-kernel,
linux-phy, p.zabel, airlied, simona, maarten.lankhorst, mripard,
tzimmermann, robh, krzk+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, tglx, vkoul, kishon, Frank.Li, lumag, aisheng.dong, agx,
u.kleine-koenig, dmitry.baryshkov
In-Reply-To: <20260714193627.GA9616@francesco-nb>
On Tue, Jul 14, 2026 at 09:36:27PM +0200, Francesco Dolcini wrote:
> Hello Liu,
Hello Francesco,
>
> On Mon, Apr 14, 2025 at 11:50:09AM +0800, Liu Ying wrote:
> > Hi,
> >
> > This patch series aims to add Freescale i.MX8qxp Display Controller support.
> >
> > The controller is comprised of three main components that include a blit
> > engine for 2D graphics accelerations, display controller for display output
> > processing, as well as a command sequencer.
>
> ...
>
> >
> > To follow up i.MX8qxp TRM, I changed the controller name to "Display Controller"
> > instead of the previous "DPU". "DPU" is only mentioned in the SoC block
> > diagram and represents the whole display subsystem which includes the display
> > controller and prefech engines, etc.
> >
> > With an additional patch[1] for simple-pm-bus.c, this series facilitates
> > testing a LVDS panel on i.MX8qxp MEK.
> >
> > Please do NOT merge patch 14-19. They are only used to facilitate testing
> > the LVDS panel.
>
> What's the plan to conclude this work? What's the latest status?
> I am looking forward to have a way to use the i.MX8QXP display with
> mainline, but to my understanding some required changes on the SOC dtsi
> are not merged.
>
> Can you help?
I planned to add display controller DT node after prefetch engine support [1]
is picked up so that the prefetch engine could be enabled together with the
display controller. Just want to avoid a DT that only enables the display
controller or bypasses the prefetch engine. It makes the display controller
driver simpler.
[1] https://lore.kernel.org/all/20251027-imx8-dc-prefetch-v5-0-4ecb6c6d4941@nxp.com/
>
> Thanks,
> Francesco
>
--
Regards,
Liu Ying
--
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: Frank Wang @ 2026-07-16 2:50 UTC (permalink / raw)
To: Sebastian Reichel, Andy Yan, William Wu, Heiko Stuebner,
Vinod Koul, Neil Armstrong, Thinh Nguyen
Cc: Greg Kroah-Hartman, Dmitry Baryshkov, Yubing Zhang,
Alexey Charkov, linux-arm-kernel, linux-rockchip, linux-kernel,
kernel, devicetree, Sashiko, linux-usb, linux-phy, wmc
In-Reply-To: <alfM6dFBovHMaYn2@venus>
Hi Sebastian,
On 2026/7/16 2:24, Sebastian Reichel wrote:
> 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.
>
One thought I had was whether you've considered splitting this patch series into several smaller, targeted series, for
instance, one covering code optimizations, and separate series for distinct groups of bug fixes. This would keep each
series lean and easier for maintainers to review.
> 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.
>
On the dwc3 core changes, based on Rockchip's internal testing and experience, runtime suspend does not kick in as
intended during hotplug events. The dwc3 core uses a default autosuspend delay of 5 seconds (defined as
DWC3_DEFAULT_AUTOSUSPEND_DELAY). I think we can try to make this parameter configurable via device tree properties, as
this approach would result in a smaller, less invasive change overall.
Best regards,
Frank
> Greetings,
>
> -- Sebastian
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v13 29/35] usb: dwc3: core: support PHY reset notifications
From: Thinh Nguyen @ 2026-07-16 0:38 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Vinod Koul, Neil Armstrong, Heiko Stuebner, Frank Wang,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Thinh Nguyen,
Greg Kroah-Hartman, Philipp Zabel, Andy Yan, Dmitry Baryshkov,
Yubing Zhang, Alexey Charkov, linux-phy@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
kernel@collabora.com, devicetree@vger.kernel.org,
linux-usb@vger.kernel.org
In-Reply-To: <20260714-rockchip-usbdp-cleanup-v13-29-6cb3e769d4c5@collabora.com>
On Tue, Jul 14, 2026, Sebastian Reichel wrote:
> 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.
>
> If the USBDP PHY resets with the DWC3 IP running, its internal state
> corrupts resulting in the USBDP PHY not being able to lock some PLL
> clocks, which effectively renders USB3 unusable.
>
> To fix the issue this adds handling for the new PHY framework reset
> notifications, which will assert PHYSOFTRST before the actual PHY
> is disabled and will deassert it once the PHY returns.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> drivers/usb/dwc3/core.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/usb/dwc3/core.h | 18 +++++++++
> 2 files changed, 119 insertions(+)
>
These changes should not be in the core. This is specific to Rockchip
and not every phy needs this. Perhaps you can create a new glue driver
for this.
Thanks,
Thinh
--
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: Coia Prant @ 2026-07-15 23:01 UTC (permalink / raw)
To: Andrew Lunn
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: <039c2cce-e76e-4321-ba4a-cd80801f136e@lunn.ch>
Andrew Lunn <andrew@lunn.ch> 于2026年7月16日周四 06:25写道：
>
> > The current design
> > ==================
> > The dependency chain I chose is simple:
> >
> > stmmac (GMAC) -> pcs-handle -> XPCS node -> phys -> SerDes PHY
>
> Maybe we need to be more accurate here.
>
> In QSGMII I _think_ it is actually
>
> stmmac (GMAC) -> pcs-handle -> XPCS node -\
> stmmac (GMAC) -> pcs-handle -> XPCS node -------> phys -> SerDes PHY
> stmmac (GMAC) -> pcs-handle -> XPCS node -/ /
> stmmac (GMAC) -> pcs-handle -> XPCS node --/
>
> There are four instances of stmmac, four instances of the XPCS, which
> then all come together into one PHY.
>
> When using SGMII, does it look like this ?
>
> stmmac (GMAC) -> pcs-handle -> XPCS node -------> phys -> SerDes PHY
> stmmac (GMAC) -> pcs-handle -> XPCS node -------> phys -> SerDes PHY
> stmmac (GMAC) -> pcs-handle -> XPCS node -------> phys -> SerDes PHY
> stmmac (GMAC) -> pcs-handle -> XPCS node -------> phys -> SerDes PHY
>
> There are four of everything, all working independently.
>
> Is there 1 QSGMII PHY and 4 SGMII PHYs? Or does one of the 4 phys
> support SGMII + QSGMII, while 3 are SGMII only?
>
> > 2. User misconfiguration: even with reference counting, if a user sets
> > `phys = <&combphy2 PHY_TYPE_SGMII>` for GMAC0 and
> > `phys = <&combphy2 PHY_TYPE_QSGMII>` for GMAC1, the same PHY would
> > be initialized in two different modes.
>
> I would hope that results in -EINVAL.
>
> Andrew
Hi Andrew,
Thanks for the follow-up. I've dug through Rockchip's BSP and found the
exact device tree configurations they use for their EVBs, which I think
clears up the topology question.
For reference, here are the relevant BSP device tree files:
- QSGMII: https://github.com/armbian/linux-rockchip/blob/rk-6.1-rkr6.1/arch/arm64/boot/dts/rockchip/rk3568-evb2-lp4x-v10.dtsi#L185
- SGMII: https://github.com/armbian/linux-rockchip/blob/rk-6.1-rkr6.1/arch/arm64/boot/dts/rockchip/rk3568-evb5-ddr4-v10.dtsi#L208
Hardware topology confirmed
===========================
The BSP shows that for RK3568, the hardware is indeed a shared resource.
One important clarification: the XPCS block has four logical entities
(corresponding to the four MII ports, pcs-mii@0..3). In my driver, these
are all attached to the same MDIO bus and distinguished by address.
To clarify the PHY topology: there is only one physical SerDes PHY on
RK3568 (combphy1_usq). In QSGMII mode, this single PHY provides four
logical ports via time-division multiplexing. In the device tree, all
four ports are attached to the same MDIO bus and distinguished by
address (pcs-mii@0..3).
So it's not "4 PHYs" but "1 PHY with 4 logical ports", all accessible
through the same MDIO bus with different address. The BSP EVB2
configuration confirms this:
both gmac0 and gmac1 reference the same combphy1_usq with
PHY_TYPE_QSGMII.
To clarify the difference between SGMII and QSGMII:
- SGMII is a single serial link at 1.25 Gbaud, providing one Gigabit port.
- QSGMII is a 5 Gbaud SerDes protocol that multiplexes four SGMII links
onto the same physical lanes.
So for RK3568: one PHY for SGMII, or one PHY for QSGMII (both GMACs share
the same PHY, each using a dedicated logical port).
For SGMII mode, only one of these ports can be used at a time, and only
one GMAC can use the SGMII link. The BSP EVB5 configuration shows this
clearly: gmac0 uses SGMII, while gmac1 falls back to RGMII.
For QSGMII mode, the BSP EVB2 configuration shows both gmac0 and gmac1
configured with phy-mode = "qsgmii", both referencing the same combphy1_usq
as their phys. This confirms your diagram: multiple MACs converge on a
single SerDes PHY via the XPCS, with the four logical ports time-division
multiplexed onto the single PHY.
Why this matters for the design
===============================
This BSP evidence supports my current design where the XPCS node owns
the SerDes PHY:
1. It matches the hardware: the XPCS is the central multiplexer.
Placing the PHY under the XPCS node in the device tree accurately
reflects this hardware reality.
2. It's the only viable path for QSGMII: as the BSP shows, QSGMII
requires multiple MACs to share one PHY. If the PHY were owned by the
MAC driver (dwmac-rk), we'd need complex refcounting and cross-MAC
coordination. The XPCS-owner model solves this cleanly by making the
XPCS the single point of control.
3. It aligns with the BSP's own logic: even though the BSP uses a
non-standard property (rockchip,xpcs), the underlying idea is the
same: the MACs point to a shared XPCS node, which then manages the
PHY. My design just does this using the standard kernel frameworks.
So, I believe the current design is correct. It's based on the hardware
reality confirmed by Rockchip's own BSP, and it uses the standard kernel
interfaces to do so. I'm happy to keep the XPCS as the owner of the
SerDes PHY in v2 if you agree.
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: Andrew Lunn @ 2026-07-15 22:25 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: <CALj3r0gBnyEVwo3rK-dKssYN9F4nFn93QJ24FVv8YBLjNNgPmg@mail.gmail.com>
> The current design
> ==================
> The dependency chain I chose is simple:
>
> stmmac (GMAC) -> pcs-handle -> XPCS node -> phys -> SerDes PHY
Maybe we need to be more accurate here.
In QSGMII I _think_ it is actually
stmmac (GMAC) -> pcs-handle -> XPCS node -\
stmmac (GMAC) -> pcs-handle -> XPCS node -------> phys -> SerDes PHY
stmmac (GMAC) -> pcs-handle -> XPCS node -/ /
stmmac (GMAC) -> pcs-handle -> XPCS node --/
There are four instances of stmmac, four instances of the XPCS, which
then all come together into one PHY.
When using SGMII, does it look like this ?
stmmac (GMAC) -> pcs-handle -> XPCS node -------> phys -> SerDes PHY
stmmac (GMAC) -> pcs-handle -> XPCS node -------> phys -> SerDes PHY
stmmac (GMAC) -> pcs-handle -> XPCS node -------> phys -> SerDes PHY
stmmac (GMAC) -> pcs-handle -> XPCS node -------> phys -> SerDes PHY
There are four of everything, all working independently.
Is there 1 QSGMII PHY and 4 SGMII PHYs? Or does one of the 4 phys
support SGMII + QSGMII, while 3 are SGMII only?
> 2. User misconfiguration: even with reference counting, if a user sets
> `phys = <&combphy2 PHY_TYPE_SGMII>` for GMAC0 and
> `phys = <&combphy2 PHY_TYPE_QSGMII>` for GMAC1, the same PHY would
> be initialized in two different modes.
I would hope that results in -EINVAL.
Andrew
--
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: Coia Prant @ 2026-07-15 22:04 UTC (permalink / raw)
To: sashiko-reviews
Cc: vkoul, neil.armstrong, linux-phy, conor+dt, devicetree, robh,
olteanv
In-Reply-To: <20260715191454.D12521F00AC4@smtp.kernel.org>
> [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
No. This change is intentional. pcs-xpcs-rk is a platform-specific
driver and should be built as a separate module, not linked into
the composite pcs_xpcs module. There are no undefined references.
> [ ... ]
>
> > 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.
No. Read remapping is performed by hardware. The VEND2 registers
reflect the settings of MII0; the corresponding bits for MII1-3 are
read-only. This is a hardware feature, not a bug.
> [ ... ]
>
> > +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.
You are correct. I followed the same pattern as pcs-xpcs-plat.c,
but I agree this could be a problem. I will add a .remove callback
in v2 that calls pm_runtime_force_suspend() before devres cleanup
runs, ensuring the clock enable count is balanced before clk_put().
> [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.
This would only happen with a misconfigured device tree. However,
for robustness, I will add a compatible string check in v2 before
dereferencing drvdata.
> [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.
No. of_find_device_by_node() increments the reference count of the
device. The supplier cannot be unbound between of_find_device_by_node()
and put_device() because the reference count is held.
> [ ... ]
>
> > +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.
You are correct. I will use pm_ptr() to wrap the PM ops in v2.
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 01/10] net: stmmac: move XPCS lifetime management to platform drivers
From: Coia Prant @ 2026-07-15 21:39 UTC (permalink / raw)
To: Andrew Lunn
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: <685d91a3-be6f-42c6-9e23-07629e21bcee@lunn.ch>
Andrew Lunn <andrew@lunn.ch> 于2026年7月16日周四 03:57写道：
>
> > 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
Hi Andrew,
Thanks for sharing this. It's reassuring to know that I'm not alone in
finding this code painful.
I fully agree that ripping it out and starting over would be a huge
undertaking, and I'm definitely not confident enough to take that on
right now - especially given the risk of breaking existing platforms.
My current focus is on getting this RK3568 SGMII support into a
mergeable state, rather than attempting a full redesign of the
stmmac PCS layer.
That said, I'll keep this in mind. Once my current series lands and I
have a better understanding of the different PCS variants, maybe I'll
be in a better position to contribute to that long-term effort.
For now, I'll stick to the incremental fix that this series provides.
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: Coia Prant @ 2026-07-15 21:23 UTC (permalink / raw)
To: Andrew Lunn
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: <1d92078f-3009-43d1-a228-c3e67a1f4f4c@lunn.ch>
Andrew Lunn <andrew@lunn.ch> 于2026年7月16日周四 03:57写道：
>
> > 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
On Thu, Jul 16, 2026 at 03:57:22AM +0200, Andrew Lunn wrote:
> > 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?
Hi Andrew,
Thanks for the thoughtful question. Let me walk through the design
constraints on RK3568 and explain why I chose the current approach.
First, I want to emphasize that this is still RFC, so the binding is not
frozen. However, DT is ABI once merged, and changing it later is painful
and often requires maintaining compatibility with old bindings. So I
would like to be careful about the architecture we settle on.
The current design
==================
The dependency chain I chose is simple:
stmmac (GMAC) -> pcs-handle -> XPCS node -> phys -> SerDes PHY
The XPCS driver is the single owner of both the SerDes PHY and the
power domain. The MAC driver doesn't need to know about them at all.
On RK3568, phy_power_on() is effectively a no-op for the Combo PHY in
SGMII mode; only phy_init() is required. So the PHY is not a
power-controlled resource — it's just a one-time initialization.
Power domain management is handled via PM runtime:
- XPCS probe: dev_pm_genpd_rpm_always_on() ensures PD_PIPE is powered
(this allows us to use PM to manage csr_clk)
- Each register access: pm_runtime_resume_and_get()/put() controls
the CSR clock
This is the simplest model that works for SGMII/QSGMII today.
Why not move SerDes to stmmac?
==============================
If we move SerDes and power domain control to the MAC driver, we
run into several problems:
1. Reference counting for phy_init()/phy_exit(): in QSGMII mode, four
MACs would all call phy_init() on the same hardware. The generic PHY
core does not currently provide reference counting for these calls.
Adding it would be a larger change affecting all PHY users.
2. User misconfiguration: even with reference counting, if a user sets
`phys = <&combphy2 PHY_TYPE_SGMII>` for GMAC0 and
`phys = <&combphy2 PHY_TYPE_QSGMII>` for GMAC1, the same PHY would
be initialized in two different modes.
3. The stmmac SerDes callbacks are legacy: the existing
serdes_poweron/serdes_powerdown hooks are called from
stmmac_legacy_serdes_power_on/down functions. They are marked as
legacy and not intended for new hardware designs. Building new
support on top of legacy APIs seems like the wrong direction.
Current QSGMII usage on RK3568
===============================
I searched OpenWrt, Armbian, ImmortalWrt, and GitHub for any actual
device using "rockchip,rk3568-xpcs" with QSGMII and found none. It seems
QSGMII on RK3568 is limited to specialized evaluation boards or niche
industrial applications. Even SGMII users are rare — most boards use
RGMII simply because the pins cannot be repurposed.
If QSGMII does become a real requirement in the future, the generic PHY
core could be extended with reference counting for phy_init()/phy_exit().
That would be a cleaner solution than pushing SerDes management into each
MAC driver, and would benefit all platforms with shared SerDes resources.
That said, I had an earlier test with serdes_poweron/serdes_powerdown
callbacks in stmmac, so if the community strongly prefers to move
SerDes control to dwmac-rk, I can do that in v2.
But my preference is to keep the current design: XPCS as the single
owner of the SerDes and power domain. It works today, keeps the DT
binding stable for future QSGMII, and avoids complexity.
Do you think the generic PHY core should grow reference counting for
phy_init()/phy_exit()? Or would you prefer to see a more generic
abstraction for shared SerDes resources?
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 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
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