* [PATCH v3 phy-next 0/2] phy: ti: add driver for TI DS125DF111 Dual-Channel Retimer
From: Ioana Ciornei @ 2026-05-16 6:03 UTC (permalink / raw)
To: vkoul, neil.armstrong, robh, krzk+dt, conor+dt, johan, linux-phy
Cc: devicetree, linux-kernel
This patch set adds a generic PHY driver and the corresponding DT
binding for the TI DS125DF111 Dual-Channel retimer. The datasheet on
which this driver was based on can be found at -
https://www.ti.com/lit/gpn/DS125DF111.
A separate generic PHY is registered for each of the two channels of the
retimer, so consumers can drive each channel independently. This allows
for independent control of the channels, which is especially important
since each channel can be routed to different SerDes lanes and it is not
guaranteed that the same retimer will do both directions of SerDes lane.
This was tested on a LS1088ARDB board with the Lynx10G SerDes PHY driver
yet to be submitted.
Changes in v3:
- Use reverse Christmas tree ordering
- Print a symbolic description in case of error
- Some words do not need to be capitalized
- Remove duplicated exit code path
- Return -EINVAL in case of unsupported submode received in .set_mode()
- Add a .validate() callback
- Remove comma after sentinel entry
- Add a ds125df111_rmw() helper
- Use read_poll_timeout() to wait for channel reset to complete
- Link to v2: https://lore.kernel.org/all/20260515110145.1925579-1-ioana.ciornei@nxp.com/
Changes in v2:
- Remove the label from the example
- Rename the node from 'retimer' to 'phy'
- Explicitly include all the needed headers
- Change ds125df111_xlate() so that it returns an error if args_count is
not exactly 1
- Add a MAINTAINERS entry
- Link to v1: https://lore.kernel.org/all/20260513185103.1371809-1-ioana.ciornei@nxp.com/
Ioana Ciornei (2):
dt-bindings: phy: add PHY bindings for the TI DS125DF111 Retimer PHY
phy: ti: add PHY driver for TI DS125DF111 Dual-Channel Retimer
.../bindings/phy/ti,ds125df111.yaml | 46 +++
MAINTAINERS | 7 +
drivers/phy/ti/Kconfig | 10 +
drivers/phy/ti/Makefile | 1 +
drivers/phy/ti/phy-ds125df111.c | 294 ++++++++++++++++++
5 files changed, 358 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/ti,ds125df111.yaml
create mode 100644 drivers/phy/ti/phy-ds125df111.c
--
2.25.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v3 1/2] phy: rockchip: inno-hdmi: Add configure() and validate() ops
From: Jonas Karlman @ 2026-05-15 21:04 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Heiko Stuebner
Cc: linux-phy, linux-rockchip, linux-arm-kernel, linux-kernel
In-Reply-To: <20260515195512.1757363-2-jonas@kwiboo.se>
Hi,
On 5/15/2026 9:55 PM, Jonas Karlman wrote:
> The commit 10ed34d6eaaf ("phy: Add HDMI configuration options")
> introduced a way for HDMI PHYs to be configured through the generic
> phy_configure() function.
>
> This driver derives the TMDS character rate from the pixel clock and the
> PHY bus width setting. However, no in-tree consumer of this PHY has ever
> called phy_set_bus_width() to change the TMDS character rate as only
> 8-bit RGB output is supported by the HDMI display driver.
>
> Add configure() and validate() ops to allow consumers to configure the
> TMDS character rate using phy_configure(). Fallback to the deprecated
> way of using the PHY bus width to configure the TMDS character rate.
>
> A typical call chain during DRM modeset on a RK3328 device:
>
> dw_hdmi_rockchip_encoder_atomic_check():
> - inno_hdmi_phy_validate(): pixclock 148500000 tmdsclock 594000000
>
> dw_hdmi_rockchip_encoder_atomic_mode_set():
> - inno_hdmi_phy_configure(): pixclock 148500000
> - inno_hdmi_phy_validate(): pixclock 148500000 tmdsclock 594000000
>
> vop_crtc_atomic_enable():
> - inno_hdmi_phy_rk3328_clk_set_rate(): rate 594000000 tmdsclk 594000000
> - inno_hdmi_phy_rk3328_clk_set_rate(): pixclock 594000000 tmdsclock 594000000
> - inno_hdmi_phy_rk3328_clk_recalc_rate(): pixclock 594000000 vco 594000000
>
> dw_hdmi_rockchip_encoder_enable():
> - inno_hdmi_phy_power_on(): Inno HDMI PHY Power On
> - inno_hdmi_phy_rk3328_clk_set_rate(): rate 594000000 tmdsclk 594000000
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> Changes in v3:
> - Change validate() ops to only validate tmdsclock
> - Add comments about expected consumer usage
> - Update commit message with a typical call chain
> Changes in v2:
> - Add validate() ops to validate that the TMDS rate is supported
> - Split out parts that remove the old workaround into a separate patch
>
> Patch "drm/rockchip: dw_hdmi: Configure HDMI PHY in atomic_mode_set()"
> at [1] adds phy_validate() and phy_configure() calls for this HDMI PHY.
>
> [1] https://lore.kernel.org/dri-devel/20260510183114.1248840-10-jonas@kwiboo.se/
> ---
> drivers/phy/rockchip/phy-rockchip-inno-hdmi.c | 60 ++++++++++++++++++-
> 1 file changed, 59 insertions(+), 1 deletion(-)
[snip]
> +static int inno_hdmi_phy_validate(struct phy *phy, enum phy_mode mode,
> + int submode, union phy_configure_opts *opts)
> +{
> + const struct pre_pll_config *cfg = pre_pll_cfg_table;
> + unsigned long tmdsclock;
> +
> + if (!(mode == PHY_MODE_HDMI && submode == PHY_HDMI_MODE_TMDS))
> + return -EINVAL;
> +
> + if (!opts->hdmi.tmds_char_rate || opts->hdmi.tmds_char_rate > 594000000)
> + return -EINVAL;
Sashiko reasoning log pointed out that a consumer of phy_validate() or
phy_configure() can make a call with opts=NULL, so I may likely send a
v4 of this series to fix such possible NULL pointer dereference here.
Or is that something that possible should be checked before phy core
calls the .validate()/.configure() ops?
Multiple other phy .configure() ops seem to dereference opts members
without any type of opts NULL check. (next-20260508)
Regards,
Jonas
[snip]
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH v3 2/2] phy: rockchip: inno-hdmi: Remove deprecated way to configure TMDS rate
From: Jonas Karlman @ 2026-05-15 19:55 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Heiko Stuebner
Cc: linux-phy, linux-rockchip, linux-arm-kernel, linux-kernel,
Jonas Karlman
In-Reply-To: <20260515195512.1757363-1-jonas@kwiboo.se>
The TMDS character rate of this PHY is configured using PHY bus width
in downstream vendor kernel and out-of-tree patches, however no in-tree
consumer of this PHY has ever called phy_set_bus_width() to change the
TMDS character rate as currently only 8-bit RGB output is supported by
the HDMI display driver.
The series "Split Generic PHY consumer and provider" clarifies that
phy_set_bus_width() is intended as a provider-only function.
Remove the deprecated unused fallback way to configure TMDS character
rate now that this HDMI PHY support using phy_configure() to configure
the TMDS character rate.
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v3: No change
v2: New patch, split from original patch
---
drivers/phy/rockchip/phy-rockchip-inno-hdmi.c | 17 +----------------
1 file changed, 1 insertion(+), 16 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
index efa95c22b3bb..3bc73f0cff70 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
@@ -555,24 +555,10 @@ static inline void inno_update_bits(struct inno_hdmi_phy *inno, u8 reg,
static unsigned long inno_hdmi_phy_get_tmdsclk(struct inno_hdmi_phy *inno,
unsigned long rate)
{
- int bus_width;
-
if (inno->hdmi_cfg.tmds_char_rate)
return inno->hdmi_cfg.tmds_char_rate;
- bus_width = phy_get_bus_width(inno->phy);
-
- switch (bus_width) {
- case 4:
- case 5:
- case 6:
- case 10:
- case 12:
- case 16:
- return (u64)rate * bus_width / 8;
- default:
- return rate;
- }
+ return rate;
}
static irqreturn_t inno_hdmi_phy_rk3328_hardirq(int irq, void *dev_id)
@@ -1450,7 +1436,6 @@ static int inno_hdmi_phy_probe(struct platform_device *pdev)
phy_set_drvdata(inno->phy, inno);
phy_set_mode_ext(inno->phy, PHY_MODE_HDMI, PHY_HDMI_MODE_TMDS);
- phy_set_bus_width(inno->phy, 8);
if (inno->plat_data->ops->init) {
ret = inno->plat_data->ops->init(inno);
--
2.54.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v3 1/2] phy: rockchip: inno-hdmi: Add configure() and validate() ops
From: Jonas Karlman @ 2026-05-15 19:55 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Heiko Stuebner
Cc: linux-phy, linux-rockchip, linux-arm-kernel, linux-kernel,
Jonas Karlman
In-Reply-To: <20260515195512.1757363-1-jonas@kwiboo.se>
The commit 10ed34d6eaaf ("phy: Add HDMI configuration options")
introduced a way for HDMI PHYs to be configured through the generic
phy_configure() function.
This driver derives the TMDS character rate from the pixel clock and the
PHY bus width setting. However, no in-tree consumer of this PHY has ever
called phy_set_bus_width() to change the TMDS character rate as only
8-bit RGB output is supported by the HDMI display driver.
Add configure() and validate() ops to allow consumers to configure the
TMDS character rate using phy_configure(). Fallback to the deprecated
way of using the PHY bus width to configure the TMDS character rate.
A typical call chain during DRM modeset on a RK3328 device:
dw_hdmi_rockchip_encoder_atomic_check():
- inno_hdmi_phy_validate(): pixclock 148500000 tmdsclock 594000000
dw_hdmi_rockchip_encoder_atomic_mode_set():
- inno_hdmi_phy_configure(): pixclock 148500000
- inno_hdmi_phy_validate(): pixclock 148500000 tmdsclock 594000000
vop_crtc_atomic_enable():
- inno_hdmi_phy_rk3328_clk_set_rate(): rate 594000000 tmdsclk 594000000
- inno_hdmi_phy_rk3328_clk_set_rate(): pixclock 594000000 tmdsclock 594000000
- inno_hdmi_phy_rk3328_clk_recalc_rate(): pixclock 594000000 vco 594000000
dw_hdmi_rockchip_encoder_enable():
- inno_hdmi_phy_power_on(): Inno HDMI PHY Power On
- inno_hdmi_phy_rk3328_clk_set_rate(): rate 594000000 tmdsclk 594000000
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v3:
- Change validate() ops to only validate tmdsclock
- Add comments about expected consumer usage
- Update commit message with a typical call chain
Changes in v2:
- Add validate() ops to validate that the TMDS rate is supported
- Split out parts that remove the old workaround into a separate patch
Patch "drm/rockchip: dw_hdmi: Configure HDMI PHY in atomic_mode_set()"
at [1] adds phy_validate() and phy_configure() calls for this HDMI PHY.
[1] https://lore.kernel.org/dri-devel/20260510183114.1248840-10-jonas@kwiboo.se/
---
drivers/phy/rockchip/phy-rockchip-inno-hdmi.c | 60 ++++++++++++++++++-
1 file changed, 59 insertions(+), 1 deletion(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
index 1483907413fa..efa95c22b3bb 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c
@@ -245,6 +245,7 @@ struct inno_hdmi_phy {
struct clk *phyclk;
unsigned long pixclock;
unsigned long tmdsclock;
+ struct phy_configure_opts_hdmi hdmi_cfg;
};
struct pre_pll_config {
@@ -554,7 +555,12 @@ static inline void inno_update_bits(struct inno_hdmi_phy *inno, u8 reg,
static unsigned long inno_hdmi_phy_get_tmdsclk(struct inno_hdmi_phy *inno,
unsigned long rate)
{
- int bus_width = phy_get_bus_width(inno->phy);
+ int bus_width;
+
+ if (inno->hdmi_cfg.tmds_char_rate)
+ return inno->hdmi_cfg.tmds_char_rate;
+
+ bus_width = phy_get_bus_width(inno->phy);
switch (bus_width) {
case 4:
@@ -602,6 +608,55 @@ static irqreturn_t inno_hdmi_phy_rk3328_irq(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static int inno_hdmi_phy_validate(struct phy *phy, enum phy_mode mode,
+ int submode, union phy_configure_opts *opts)
+{
+ const struct pre_pll_config *cfg = pre_pll_cfg_table;
+ unsigned long tmdsclock;
+
+ if (!(mode == PHY_MODE_HDMI && submode == PHY_HDMI_MODE_TMDS))
+ return -EINVAL;
+
+ if (!opts->hdmi.tmds_char_rate || opts->hdmi.tmds_char_rate > 594000000)
+ return -EINVAL;
+
+ /*
+ * phy_validate() is expected to be called from encoder atomic_check(),
+ * before the hdmiphy pixel clock is known. Without knowing the actual
+ * pixel clock, we cannot do full validation of the configuration.
+ * Instead, we do a simple check that the pre-pll table contains an
+ * entry for the requested TMDS character rate.
+ */
+ tmdsclock = opts->hdmi.tmds_char_rate;
+ for (; cfg->pixclock != 0; cfg++)
+ if (cfg->tmdsclock == tmdsclock)
+ return 0;
+
+ return -EINVAL;
+}
+
+static int inno_hdmi_phy_configure(struct phy *phy,
+ union phy_configure_opts *opts)
+{
+ struct inno_hdmi_phy *inno = phy_get_drvdata(phy);
+ int ret;
+
+ ret = inno_hdmi_phy_validate(phy, phy_get_mode(phy),
+ PHY_HDMI_MODE_TMDS, opts);
+ if (ret)
+ return ret;
+
+ /*
+ * phy_configure() is expected to be called from atomic_set_mode(),
+ * before the hdmiphy pixel clock is known. Store the requested TMDS
+ * character rate, so that it can be used later in power_on() and/or
+ * set_rate() when the pixel clock is known.
+ */
+ inno->hdmi_cfg = opts->hdmi;
+
+ return 0;
+}
+
static int inno_hdmi_phy_power_on(struct phy *phy)
{
struct inno_hdmi_phy *inno = phy_get_drvdata(phy);
@@ -670,6 +725,8 @@ static const struct phy_ops inno_hdmi_phy_ops = {
.owner = THIS_MODULE,
.power_on = inno_hdmi_phy_power_on,
.power_off = inno_hdmi_phy_power_off,
+ .configure = inno_hdmi_phy_configure,
+ .validate = inno_hdmi_phy_validate,
};
static const
@@ -1392,6 +1449,7 @@ static int inno_hdmi_phy_probe(struct platform_device *pdev)
}
phy_set_drvdata(inno->phy, inno);
+ phy_set_mode_ext(inno->phy, PHY_MODE_HDMI, PHY_HDMI_MODE_TMDS);
phy_set_bus_width(inno->phy, 8);
if (inno->plat_data->ops->init) {
--
2.54.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v2 0/2] phy: rockchip: inno-hdmi: Change TMDS rate handling to configure() ops
From: Jonas Karlman @ 2026-05-15 19:55 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Heiko Stuebner
Cc: linux-phy, linux-rockchip, linux-arm-kernel, linux-kernel,
Jonas Karlman
This series adds support for using phy_validate() and phy_configure()
with this HDMI PHY as an alternative to current in-tree unused way of
using PHY bus width to configure the TMDS character rate.
The only known users that calls phy_set_bus_width() on this PHY are my
out-of-tree HDMI 2.0 patches for Rockchip RK3228/RK3328, i.e. those
originating from LibreELEC (also carried by other distros), the
downstream vendor kernel uses a different implementation that also calls
phy_set_bus_width() on this PHY.
Patch "drm/rockchip: dw_hdmi: Configure HDMI PHY in atomic_mode_set()"
that calls phy_validate() and phy_configure() on this PHY can be found
at [1].
[1] https://lore.kernel.org/dri-devel/20260510183114.1248840-10-jonas@kwiboo.se/
This series is part of a larger multi series effort to:
- phy: rockchip: inno-hdmi: Change TMDS rate handling to configure() ops [v3]
- drm/rockchip: dw_hdmi: Misc cleanup and propagate bus format [v1]
- drm: bridge: dw_hdmi: Misc enable/disable, CEC and EDID cleanup [v5]
- drm/bridge: dw-hdmi: Improve input/output bus format handling
- drm/bridge: dw-hdmi: Convert to a HDMI bridge and use of bridge connector
- drm/bridge: dw-hdmi: Add and use tmds_char_rate_valid() plat data ops
- drm/meson: hdmi: Misc cleanup and use CEC notifier helpers
- drm/rockchip: dw_hdmi: Enable YCbCr and Deep Color modes
Link to snapshot: https://github.com/Kwiboo/linux-rockchip/commits/next-20260508-rk-hdmi-v4/
Changes in v3:
- Change validate() ops to only validate tmdsclock
- Add comments about expected consumer usage
- Update commit message with a typical call chain
Link to v2: https://lore.kernel.org/linux-phy/20260510095731.1222705-1-jonas@kwiboo.se/
Changes in v2:
- Split into two patches, one that adds new ops and a second that remove
the old and unused workaround
- Add validate() ops to validate that the TMDS rate is supported
Link to v1: https://lore.kernel.org/linux-phy/20260503172936.194003-1-jonas@kwiboo.se/
Jonas Karlman (2):
phy: rockchip: inno-hdmi: Add configure() and validate() ops
phy: rockchip: inno-hdmi: Remove deprecated way to configure TMDS rate
drivers/phy/rockchip/phy-rockchip-inno-hdmi.c | 71 +++++++++++++++----
1 file changed, 57 insertions(+), 14 deletions(-)
--
2.54.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH v3] net: phy: sfp: probe for RollBall I2C-to-MDIO bridge in mdio-i2c
From: Petr Wozniak @ 2026-05-15 17:40 UTC (permalink / raw)
To: netdev; +Cc: bjorn, andrew, linux-phy, Petr Wozniak
The "OEM"/"SFP-10G-T" quirk entry in sfp_fixup_rollball_cc()
unconditionally forces MDIO_I2C_ROLLBALL for all modules matching that
vendor/part-number combination. This works for modules that genuinely
implement a RollBall I2C-to-MDIO bridge, but silently breaks modules
that share the same EEPROM strings without having such a bridge.
The Realtek RTL8261BE-CG is one such module: a pure copper 10G SFP+
media converter with no I2C-to-MDIO bridge. Its EEPROM reports
vendor="OEM", part="SFP-10G-T-I", and -- critically -- Vendor OUI
00:00:00, making OUI-based differentiation impossible. With
MDIO_I2C_ROLLBALL the kernel stalls waiting for a PHY that never
appears:
sfp sfp2: probing phy device through the [MDIO_I2C_ROLLBALL] protocol
Move the probe into i2c_mii_init_rollball() in mdio-i2c.c, where the
RollBall protocol constants are already defined. After sending the
unlock password, issue a CMD_READ and wait ~70 ms for CMD_DONE. A
genuine RollBall bridge asserts CMD_DONE within that window; modules
without a bridge never do, so i2c_mii_init_rollball() returns -ENODEV.
sfp_i2c_mdiobus_create() treats -ENODEV as no PHY and falls back to
MDIO_I2C_NONE without creating an MDIO bus.
Add "OEM"/"SFP-10G-T-I" to the quirk table so RTL8261BE modules enter
the probe path; genuine RollBall modules continue to work as before.
Signed-off-by: Petr Wozniak <petr.wozniak@gmail.com>
Tested-by: Petr Wozniak <petr.wozniak@gmail.com>
---
Targeting: net-next
Changes since v2:
- Compile-tested and hardware-tested on BPI-R4 (MT7988A, 6.12.87)
- RTL8261BE (OEM/SFP-10G-T-I): probes MDIO_I2C_NONE, link Up 10Gbps
- Genuine RollBall (OEM/SFP-10G-T): bridge detected, link Up 10Gbps
drivers/net/mdio/mdio-i2c.c | 59 +++++++++++++++++++++++++++++++++----
drivers/net/phy/sfp.c | 8 ++++-
2 files changed, 60 insertions(+), 7 deletions(-)
diff --git a/drivers/net/mdio/mdio-i2c.c b/drivers/net/mdio/mdio-i2c.c
index da2001e..1973fda 100644
--- a/drivers/net/mdio/mdio-i2c.c
+++ b/drivers/net/mdio/mdio-i2c.c
@@ -352,6 +352,52 @@ static int i2c_mii_write_rollball(struct mii_bus *bus, int phy_id, int devad,
return 0;
}
+/*
+ * Probe for a RollBall I2C-to-MDIO bridge by issuing CMD_READ and waiting
+ * ~70 ms for CMD_DONE. Genuine RollBall bridges respond within that window.
+ * Modules that share the same EEPROM vendor/part strings but have no bridge
+ * (e.g. RTL8261BE pure copper media converter) never assert CMD_DONE, so
+ * -ENODEV is returned for them.
+ */
+static int i2c_mii_probe_rollball(struct i2c_adapter *i2c)
+{
+ u8 data_buf[] = { ROLLBALL_DATA_ADDR, 0x01, 0x00, 0x00 };
+ u8 cmd_buf[] = { ROLLBALL_CMD_ADDR, ROLLBALL_CMD_READ };
+ u8 cmd_addr = ROLLBALL_CMD_ADDR, result;
+ struct i2c_msg msgs[2];
+ int ret;
+
+ msgs[0].addr = ROLLBALL_PHY_I2C_ADDR;
+ msgs[0].flags = 0;
+ msgs[0].len = sizeof(data_buf);
+ msgs[0].buf = data_buf;
+ msgs[1].addr = ROLLBALL_PHY_I2C_ADDR;
+ msgs[1].flags = 0;
+ msgs[1].len = sizeof(cmd_buf);
+ msgs[1].buf = cmd_buf;
+
+ ret = i2c_transfer_rollball(i2c, msgs, ARRAY_SIZE(msgs));
+ if (ret)
+ return ret;
+
+ msleep(70);
+
+ msgs[0].addr = ROLLBALL_PHY_I2C_ADDR;
+ msgs[0].flags = 0;
+ msgs[0].len = 1;
+ msgs[0].buf = &cmd_addr;
+ msgs[1].addr = ROLLBALL_PHY_I2C_ADDR;
+ msgs[1].flags = I2C_M_RD;
+ msgs[1].len = 1;
+ msgs[1].buf = &result;
+
+ ret = i2c_transfer_rollball(i2c, msgs, ARRAY_SIZE(msgs));
+ if (ret)
+ return ret;
+
+ return result == ROLLBALL_CMD_DONE ? 0 : -ENODEV;
+}
+
static int i2c_mii_init_rollball(struct i2c_adapter *i2c)
{
struct i2c_msg msg;
@@ -372,10 +418,10 @@ static int i2c_mii_init_rollball(struct i2c_adapter *i2c)
ret = i2c_transfer(i2c, &msg, 1);
if (ret < 0)
return ret;
- else if (ret != 1)
+ if (ret != 1)
return -EIO;
- else
- return 0;
+
+ return i2c_mii_probe_rollball(i2c);
}
struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c,
@@ -399,9 +445,10 @@ struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c,
case MDIO_I2C_ROLLBALL:
ret = i2c_mii_init_rollball(i2c);
if (ret < 0) {
- dev_err(parent,
- "Cannot initialize RollBall MDIO I2C protocol: %d\n",
- ret);
+ if (ret != -ENODEV)
+ dev_err(parent,
+ "Cannot initialize RollBall MDIO I2C protocol: %d\n",
+ ret);
mdiobus_free(mii);
return ERR_PTR(ret);
}
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 218c775..ce745b6 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -599,7 +599,8 @@ static const struct sfp_quirk sfp_quirks[] = {
SFP_QUIRK_S("TP-LINK", "TL-SM410U", sfp_quirk_oem_2_5g),
SFP_QUIRK_F("ETU", "ESP-T5-R", sfp_fixup_rollball_cc),
- SFP_QUIRK_F("OEM", "SFP-10G-T", sfp_fixup_rollball_cc),
+ SFP_QUIRK_F("OEM", "SFP-10G-T-I", sfp_fixup_rollball),
+ SFP_QUIRK_F("OEM", "SFP-10G-T", sfp_fixup_rollball_cc),
SFP_QUIRK_S("OEM", "SFP-2.5G-T", sfp_quirk_oem_2_5g),
SFP_QUIRK_S("OEM", "SFP-2.5G-BX10-D", sfp_quirk_2500basex),
SFP_QUIRK_S("OEM", "SFP-2.5G-BX10-U", sfp_quirk_2500basex),
@@ -802,6 +803,11 @@ static int sfp_i2c_mdiobus_create(struct sfp *sfp)
int ret;
i2c_mii = mdio_i2c_alloc(sfp->dev, sfp->i2c, sfp->mdio_protocol);
+ if (PTR_ERR_OR_ZERO(i2c_mii) == -ENODEV) {
+ /* RollBall probe found no bridge: no PHY on this module */
+ sfp->mdio_protocol = MDIO_I2C_NONE;
+ return 0;
+ }
if (IS_ERR(i2c_mii))
return PTR_ERR(i2c_mii);
--
2.51.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* Re: [PATCH] net: phy: sfp: probe for RollBall I2C-to-MDIO bridge in mdio-i2c
From: Andrew Lunn @ 2026-05-15 16:44 UTC (permalink / raw)
To: Petr Wozniak; +Cc: netdev, bjorn, linux-phy
In-Reply-To: <20260515162232.23758-1-petr.wozniak@gmail.com>
On Fri, May 15, 2026 at 06:22:32PM +0200, Petr Wozniak wrote:
> I realise I sent v2 without testing it on hardware first — apologies.
> I will test and follow up with a v3 with Tested-by once verified.
Please take a read of
https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html
https://docs.kernel.org/process/submitting-patches.html
You need to include a version number in the Subject line, and indicate
which tree this patch is for.
Also, please start a new thread for every version of the patch.
Andrew
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH] net: phy: sfp: probe for RollBall I2C-to-MDIO bridge in mdio-i2c
From: Petr Wozniak @ 2026-05-15 16:22 UTC (permalink / raw)
To: andrew; +Cc: netdev, bjorn, linux-phy, Petr Wozniak
In-Reply-To: <20260515161834.23628-1-petr.wozniak@gmail.com>
I realise I sent v2 without testing it on hardware first — apologies.
I will test and follow up with a v3 with Tested-by once verified.
Petr
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH] net: phy: sfp: probe for RollBall I2C-to-MDIO bridge in mdio-i2c
From: Petr Wozniak @ 2026-05-15 16:18 UTC (permalink / raw)
To: netdev; +Cc: bjorn, andrew, linux-phy, Petr Wozniak
In-Reply-To: <20260515115527.17241-1-petr.wozniak@gmail.com>
The "OEM"/"SFP-10G-T" quirk entry in sfp_fixup_rollball_cc()
unconditionally forces MDIO_I2C_ROLLBALL for all modules matching that
vendor/part-number combination. This works for modules that genuinely
implement a RollBall I2C-to-MDIO bridge, but silently breaks modules
that share the same EEPROM strings without having such a bridge.
The Realtek RTL8261BE-CG is one such module: a pure copper 10G SFP+
media converter with no I2C-to-MDIO bridge. Its EEPROM reports
vendor="OEM", part="SFP-10G-T-I", and -- critically -- Vendor OUI
00:00:00, making OUI-based differentiation impossible. With
MDIO_I2C_ROLLBALL the kernel stalls waiting for a PHY that never
appears:
sfp sfp2: probing phy device through the [MDIO_I2C_ROLLBALL] protocol
Move the probe into i2c_mii_init_rollball() in mdio-i2c.c, where the
RollBall protocol constants are already defined. After sending the
unlock password, issue a CMD_READ and wait ~70 ms for CMD_DONE. A
genuine RollBall bridge asserts CMD_DONE within that window; modules
without a bridge never do, so i2c_mii_init_rollball() returns -ENODEV.
sfp_i2c_mdiobus_create() treats -ENODEV as no PHY and falls back to
MDIO_I2C_NONE without creating an MDIO bus.
Add "OEM"/"SFP-10G-T-I" to the quirk table so RTL8261BE modules enter
the probe path; genuine RollBall modules continue to work as before.
Signed-off-by: Petr Wozniak <petr.wozniak@gmail.com>
---
Changes since v1:
- Moved probe from sfp.c into i2c_mii_probe_rollball() in mdio-i2c.c;
all RollBall protocol logic now lives in one place.
- Removed duplicated #define constants from sfp.c.
- sfp_i2c_mdiobus_create() handles -ENODEV from mdio_i2c_alloc()
and falls back to MDIO_I2C_NONE instead of failing the module probe.
- OEM/SFP-10G-T-I uses sfp_fixup_rollball (no CC byte correction
needed for a media converter).
drivers/net/mdio/mdio-i2c.c | 59 +++++++++++++++++++++++++++++++++----
drivers/net/phy/sfp.c | 8 ++++-
2 files changed, 60 insertions(+), 7 deletions(-)
diff --git a/drivers/net/mdio/mdio-i2c.c b/drivers/net/mdio/mdio-i2c.c
index da2001e..1973fda 100644
--- a/drivers/net/mdio/mdio-i2c.c
+++ b/drivers/net/mdio/mdio-i2c.c
@@ -352,6 +352,52 @@ static int i2c_mii_write_rollball(struct mii_bus *bus, int phy_id, int devad,
return 0;
}
+/*
+ * Probe for a RollBall I2C-to-MDIO bridge by issuing CMD_READ and waiting
+ * ~70 ms for CMD_DONE. Genuine RollBall bridges respond within that window.
+ * Modules that share the same EEPROM vendor/part strings but have no bridge
+ * (e.g. RTL8261BE pure copper media converter) never assert CMD_DONE, so
+ * -ENODEV is returned for them.
+ */
+static int i2c_mii_probe_rollball(struct i2c_adapter *i2c)
+{
+ u8 data_buf[] = { ROLLBALL_DATA_ADDR, 0x01, 0x00, 0x00 };
+ u8 cmd_buf[] = { ROLLBALL_CMD_ADDR, ROLLBALL_CMD_READ };
+ u8 cmd_addr = ROLLBALL_CMD_ADDR, result;
+ struct i2c_msg msgs[2];
+ int ret;
+
+ msgs[0].addr = ROLLBALL_PHY_I2C_ADDR;
+ msgs[0].flags = 0;
+ msgs[0].len = sizeof(data_buf);
+ msgs[0].buf = data_buf;
+ msgs[1].addr = ROLLBALL_PHY_I2C_ADDR;
+ msgs[1].flags = 0;
+ msgs[1].len = sizeof(cmd_buf);
+ msgs[1].buf = cmd_buf;
+
+ ret = i2c_transfer_rollball(i2c, msgs, ARRAY_SIZE(msgs));
+ if (ret)
+ return ret;
+
+ msleep(70);
+
+ msgs[0].addr = ROLLBALL_PHY_I2C_ADDR;
+ msgs[0].flags = 0;
+ msgs[0].len = 1;
+ msgs[0].buf = &cmd_addr;
+ msgs[1].addr = ROLLBALL_PHY_I2C_ADDR;
+ msgs[1].flags = I2C_M_RD;
+ msgs[1].len = 1;
+ msgs[1].buf = &result;
+
+ ret = i2c_transfer_rollball(i2c, msgs, ARRAY_SIZE(msgs));
+ if (ret)
+ return ret;
+
+ return result == ROLLBALL_CMD_DONE ? 0 : -ENODEV;
+}
+
static int i2c_mii_init_rollball(struct i2c_adapter *i2c)
{
struct i2c_msg msg;
@@ -372,10 +418,10 @@ static int i2c_mii_init_rollball(struct i2c_adapter *i2c)
ret = i2c_transfer(i2c, &msg, 1);
if (ret < 0)
return ret;
- else if (ret != 1)
+ if (ret != 1)
return -EIO;
- else
- return 0;
+
+ return i2c_mii_probe_rollball(i2c);
}
struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c,
@@ -399,9 +445,10 @@ struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c,
case MDIO_I2C_ROLLBALL:
ret = i2c_mii_init_rollball(i2c);
if (ret < 0) {
- dev_err(parent,
- "Cannot initialize RollBall MDIO I2C protocol: %d\n",
- ret);
+ if (ret != -ENODEV)
+ dev_err(parent,
+ "Cannot initialize RollBall MDIO I2C protocol: %d\n",
+ ret);
mdiobus_free(mii);
return ERR_PTR(ret);
}
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 218c775..ce745b6 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -599,7 +599,8 @@ static const struct sfp_quirk sfp_quirks[] = {
SFP_QUIRK_S("TP-LINK", "TL-SM410U", sfp_quirk_oem_2_5g),
SFP_QUIRK_F("ETU", "ESP-T5-R", sfp_fixup_rollball_cc),
- SFP_QUIRK_F("OEM", "SFP-10G-T", sfp_fixup_rollball_cc),
+ SFP_QUIRK_F("OEM", "SFP-10G-T-I", sfp_fixup_rollball),
+ SFP_QUIRK_F("OEM", "SFP-10G-T", sfp_fixup_rollball_cc),
SFP_QUIRK_S("OEM", "SFP-2.5G-T", sfp_quirk_oem_2_5g),
SFP_QUIRK_S("OEM", "SFP-2.5G-BX10-D", sfp_quirk_2500basex),
SFP_QUIRK_S("OEM", "SFP-2.5G-BX10-U", sfp_quirk_2500basex),
@@ -802,6 +803,11 @@ static int sfp_i2c_mdiobus_create(struct sfp *sfp)
int ret;
i2c_mii = mdio_i2c_alloc(sfp->dev, sfp->i2c, sfp->mdio_protocol);
+ if (PTR_ERR_OR_ZERO(i2c_mii) == -ENODEV) {
+ /* RollBall probe found no bridge: no PHY on this module */
+ sfp->mdio_protocol = MDIO_I2C_NONE;
+ return 0;
+ }
if (IS_ERR(i2c_mii))
return PTR_ERR(i2c_mii);
--
2.51.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* Re: [PATCH 2/2] phy: ti-pipe3: Fix EPROBE_DEFER handling for clock resources
From: Vladimir Oltean @ 2026-05-15 15:58 UTC (permalink / raw)
To: Hongling Zeng
Cc: vkoul, neil.armstrong, johan, kishon, rogerq, linux-phy,
linux-kernel, zhongling0719
In-Reply-To: <20260515040737.28075-1-zenghongling@kylinos.cn>
On Fri, May 15, 2026 at 12:07:37PM +0800, Hongling Zeng wrote:
> ti_pipe3_get_clk() has two issues with -EPROBE_DEFER error handling:
>
> 1. When devm_clk_get() for sysclk fails, the function returns -EINVAL
> instead of propagating the actual error code. This masks -EPROBE_DEFER
> to -EINVAL, breaking the probe deferral mechanism and causing permanent
> driver initialization failure on systems with non-deterministic probe
> ordering.
>
> 2. For SATA PHY refclk, the function ignores all errors to support older
> DTBs missing the refclk property. However, this incorrectly ignores
> -EPROBE_DEFER as well, causing the driver to proceed without waiting
> for the clock provider to become available.
>
> Fix both issues:
> - Return PTR_ERR(phy->sys_clk) instead of -EINVAL to propagate all
> error codes including -EPROBE_DEFER
> - For SATA refclk, only ignore -ENOENT (clock not found in old DTBs)
> but propagate other errors like -EPROBE_DEFER
>
> Fixes: a70143bbef6b ("drivers: phy: usb3/pipe3: Adapt pipe3 driver to Generic PHY Framework")
> Fixes: 7f33912d2978 ("phy: ti-pipe3: Fix SATA across suspend/resume")
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> ---
> drivers/phy/ti/phy-ti-pipe3.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index 58fbc3b27813..2bfcd0c70abd 100644
> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c
> @@ -604,15 +604,22 @@ static int ti_pipe3_get_clk(struct ti_pipe3 *phy)
> {
> struct clk *clk;
> struct device *dev = phy->dev;
> + int ret;
>
> phy->refclk = devm_clk_get(dev, "refclk");
When phy->mode == PIPE3_MODE_SATA, I think it would be a good idea to
call devm_clk_get_optional() which converts ENOENT to 0 for you.
Otherwise call devm_clk_get(). In both cases, you can propagate the
returned error code without special-casing anything.
> if (IS_ERR(phy->refclk)) {
> - dev_err(dev, "unable to get refclk\n");
> + ret = PTR_ERR(phy->refclk);
> /* older DTBs have missing refclk in SATA PHY
> - * so don't bail out in case of SATA PHY.
> + * so don't bail out for -ENOENT, but still defer
> + * probe for other errors like -EPROBE_DEFER.
> */
> - if (phy->mode != PIPE3_MODE_SATA)
> - return PTR_ERR(phy->refclk);
> + if (ret == -ENOENT) {
> + if (phy->mode != PIPE3_MODE_SATA)
> + return ret;
> + } else {
> + dev_err(dev, "unable to get refclk\n");
> + return ret;
> + }
> }
>
> if (phy->mode != PIPE3_MODE_SATA) {
> @@ -629,7 +636,7 @@ static int ti_pipe3_get_clk(struct ti_pipe3 *phy)
> phy->sys_clk = devm_clk_get(dev, "sysclk");
> if (IS_ERR(phy->sys_clk)) {
> dev_err(dev, "unable to get sysclk\n");
> - return -EINVAL;
> + return PTR_ERR(phy->sys_clk);
> }
> }
>
> --
> 2.25.1
>
>
Because of the broken threading this patch can't be applied anyway, so
pw-bot: cr
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 1/2 v3] phy: ti: pipe3: Fix clock resource leak on probe errors
From: Vladimir Oltean @ 2026-05-15 15:55 UTC (permalink / raw)
To: Hongling Zeng
Cc: vkoul, neil.armstrong, johan, kishon, linux-phy, linux-kernel,
zhongling0719
In-Reply-To: <20260515040522.27793-1-zenghongling@kylinos.cn>
On Fri, May 15, 2026 at 12:05:22PM +0800, Hongling Zeng wrote:
> When devm_phy_create() or devm_of_phy_provider_register() fails,
> the refclk that was enabled earlier is not disabled, causing a
> resource leak.
>
> Fix this by adding an error handling path to disable the clock
> when these functions fail.
>
> Fixes: 234738ea3390 ("phy: ti-pipe3: move clk initialization to a separate function")
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
>
Your patch numbering broke Patchwork. Please do not name patches 1/2 if
2/2 is not in the same thread. Neither this patch nor "[2/2] phy:
ti-pipe3: Fix EPROBE_DEFER handling for clock resources" can be applied.
Also, wasn't v3 this? https://patchwork.kernel.org/project/linux-phy/patch/20260515021635.13444-1-zenghongling@kylinos.cn/
shouldn't this be v4?
pw-bot: cr
> ---
> Change in v2:
> -Add pm_runtime_disable() in error path (reported by Sashiko AI).
> ---
> change in v3:
> -Fix unbalanced clock disable by checking clk_prepare_enable()return value and
> setting sata_refclk_enabledonly on success.
> -Fix error path teardown order to match ti_pipe3_remove()(disable clock before
> disabling runtime PM).
> ---
> drivers/phy/ti/phy-ti-pipe3.c | 26 ++++++++++++++++++++++----
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index b5543b5c674c..58fbc3b27813 100644
> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c
> @@ -831,21 +831,39 @@ static int ti_pipe3_probe(struct platform_device *pdev)
> */
> if (phy->mode == PIPE3_MODE_SATA) {
> if (!IS_ERR(phy->refclk)) {
> - clk_prepare_enable(phy->refclk);
> + ret = clk_prepare_enable(phy->refclk);
> + if (ret) {
> + dev_err(dev, "Failed to enable refclk %d\n", ret);
> + goto err_pm_disable;
> + }
> phy->sata_refclk_enabled = true;
> }
> }
>
> generic_phy = devm_phy_create(dev, NULL, &ops);
> - if (IS_ERR(generic_phy))
> - return PTR_ERR(generic_phy);
> + if (IS_ERR(generic_phy)) {
> + ret = PTR_ERR(generic_phy);
> + goto err_clk_disable;
> + }
>
> phy_set_drvdata(generic_phy, phy);
>
> ti_pipe3_power_off(generic_phy);
>
> phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> - return PTR_ERR_OR_ZERO(phy_provider);
> + if (IS_ERR(phy_provider)) {
> + ret = PTR_ERR(phy_provider);
> + goto err_clk_disable;
> + }
> +
> + return 0;
> +
> +err_clk_disable:
> + if (phy->sata_refclk_enabled && !IS_ERR(phy->refclk))
> + clk_disable_unprepare(phy->refclk);
> +err_pm_disable:
> + pm_runtime_disable(dev);
> + return ret;
> }
>
> static void ti_pipe3_remove(struct platform_device *pdev)
> --
> 2.25.1
>
>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v2 2/4] dt-bindings: phy: qcom,msm8998-qmp-usb3-phy: Add support for Shikra
From: Krzysztof Kozlowski @ 2026-05-15 15:39 UTC (permalink / raw)
To: Krishna Kurapati
Cc: Neil Armstrong, Vinod Koul, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Konrad Dybcio, Xiangxu Yin,
Johan Hovold, Loic Poulain, Kathiravan Thirumoorthy,
Dmitry Baryshkov, linux-arm-msm, linux-phy, devicetree,
linux-kernel
In-Reply-To: <34586ed6-4f78-490f-a916-baf7657cca7a@oss.qualcomm.com>
On 15/05/2026 17:36, Krishna Kurapati wrote:
>
>
> On 5/14/2026 8:07 PM, Krzysztof Kozlowski wrote:
>> On 14/05/2026 08:22, Krishna Kurapati wrote:
>>>
>>>
>>> On 5/14/2026 12:26 AM, Krzysztof Kozlowski wrote:
>>>> On 07/05/2026 13:37, Krishna Kurapati wrote:
>>>>>
>>>>>
>>>>> On 5/5/2026 7:30 PM, Krzysztof Kozlowski wrote:
>>>>>> On 05/05/2026 15:57, Krishna Kurapati wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 5/5/2026 6:59 PM, Krzysztof Kozlowski wrote:
>>>>>>>> On 05/05/2026 15:27, Krishna Kurapati wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 5/5/2026 4:22 PM, Krzysztof Kozlowski wrote:
>>>>>>>>>> On 05/05/2026 12:49, Krzysztof Kozlowski wrote:
>>>>>>>>>>> On Mon, May 04, 2026 at 10:36:57PM +0530, Krishna Kurapati wrote:
>>>>>>>>>>>> Declare the USB-C QMP PHY present on the Qualcomm Shikra platform.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>> .../devicetree/bindings/phy/qcom,msm8998-qmp-usb3-phy.yaml | 2 ++
>>>>>>>>>>>> 1 file changed, 2 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
>>>>>>>>>>
>>>>>>>>>> ... and then I looked at the driver. So un-reviewed. Devices are clearly
>>>>>>>>>> compatible. If not, explain what is not compatible.
>>>>>>>>>>
>>>>>>>>> Talos uses GCC_USB3_PRIM_PHY_AUX_CLK.
>>>>>>>>>
>>>>>>>>> In Shikra, we are using GCC_USB3_PRIM_PHY_COM_AUX_CLK. We don't have
>>>>>>>>> GCC_USB3_PRIM_PHY_AUX_CLK.
>>>>>>>>>
>>>>>>>>> Hence, I didn't use a fallback compatible.
>>>>>>>>
>>>>>>>> This still explains nothing. How different clock makes interface for SW
>>>>>>>> incompatible exactly?
>>>>>>>>
>>>>>>> So I went by the naming. AUX vs COM_AUX.
>>>>>>
>>>>>> The naming does not matter. If the clock is called
>>>>>> "no_one_expects_spanish_inquisition", does that make software
>>>>>> incompatible? Why would the name itself matter?
>>>>>>
>>>>>>>
>>>>>>> Can I use a fallback compatible and in DT vote for "COM_AUX" clock with
>>>>>>> clock-names mentioning "aux" ?
>>>>>>
>>>>>> I don't know, I asked what is different in software interface.
>>>>>>
>>>>>
>>>>> Hi Krzysztof,
>>>>>
>>>>> I checked with the hw team here and found out two things.
>>>>>
>>>>> 1. Shikra is a spinoff of Agatti and its sw interface (clocks used and
>>>>> regulators used) is the same as agatti.
>>>>>
>>>>> 2. I thought we could use qcm2290 as a fallback since the phy register
>>>>> init sequence is the same for Talos/Shikra/Agatti. The difference
>>>>> between Talos and agatti when checked in the driver was the init load
>>>>> settings. I checked with the hw team and they suggested using the init
>>>>> load settings which talos was using.
>>>>>
>>>>> Hence both these compatibles (qcm2290 and qcs615) cannot be used as
>>>>> fallback for Shikra.
>>>>
>>>> Then I do not understand why you are using qcs615_usb3phy_cfg for
>>>> Shikra. You say that the initialization is different, but you use
>>>> exactly the same initialization. So in a meaning of compatibility
>>>> between hardware for Devicetree they are compatible.
>>>>
>>> Hi Krzysztof,
>>>
>>> There are 3 things:
>>>
>>> 1. Clocks used:
>>> -> Talos supports AUX Clock since it supports DP over USB.
>>> -> Agatti and Shikra use COM_AUX clock since they dont support DP over USB.
>>>
>>> 2. Phy register Init sequence - same for all 3 targets
>>>
>>> 3. Regulator init load:
>>> -> Different for both Talos and Agatti
>>> -> Recommendation is to use Talos regulator load values.
>>>
>>> SW interface wise, shikra is comaptible with agatti. If we use agatti as
>>> fallback, we would end up using the platform data of Agatti where the
>>> regulator init load is not suitable for Shikra. Hence not using Agatti
>>> as fallback.
>>>
>>> Coming to driver changes, I used qcs615_cfg because it has required phy
>>> register sequence and regulator init load as needed by shikra.
>>
>> So is it compatible with QCS615? If not, then something is incomplete or
>> confusing. The driver uses the same software interface.
>>
> Sorry for the confusion. The Talos compatible represents the USB/DP PHY
> with aux clock input, while Shikra is a USB-only PHY with com_aux input
> clock, so the two PHYs are not compatible with each other.
>
> In the Linux driver implementation the match data is currently used only
> to affect the init sequence and regulator init load and here Shikra can
> reuse the Talos match data structure.
>
I wrote three times already, not writing anymore. You still did not
answer the problem here.
Best regards,
Krzysztof
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v2 2/4] dt-bindings: phy: qcom,msm8998-qmp-usb3-phy: Add support for Shikra
From: Krishna Kurapati @ 2026-05-15 15:36 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Neil Armstrong, Vinod Koul, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Konrad Dybcio, Xiangxu Yin,
Johan Hovold, Loic Poulain, Kathiravan Thirumoorthy,
Dmitry Baryshkov, linux-arm-msm, linux-phy, devicetree,
linux-kernel
In-Reply-To: <9ed7c714-07c1-48de-8d27-cbe24356c606@kernel.org>
On 5/14/2026 8:07 PM, Krzysztof Kozlowski wrote:
> On 14/05/2026 08:22, Krishna Kurapati wrote:
>>
>>
>> On 5/14/2026 12:26 AM, Krzysztof Kozlowski wrote:
>>> On 07/05/2026 13:37, Krishna Kurapati wrote:
>>>>
>>>>
>>>> On 5/5/2026 7:30 PM, Krzysztof Kozlowski wrote:
>>>>> On 05/05/2026 15:57, Krishna Kurapati wrote:
>>>>>>
>>>>>>
>>>>>> On 5/5/2026 6:59 PM, Krzysztof Kozlowski wrote:
>>>>>>> On 05/05/2026 15:27, Krishna Kurapati wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 5/5/2026 4:22 PM, Krzysztof Kozlowski wrote:
>>>>>>>>> On 05/05/2026 12:49, Krzysztof Kozlowski wrote:
>>>>>>>>>> On Mon, May 04, 2026 at 10:36:57PM +0530, Krishna Kurapati wrote:
>>>>>>>>>>> Declare the USB-C QMP PHY present on the Qualcomm Shikra platform.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>
>>>>>>>>>>> ---
>>>>>>>>>>> .../devicetree/bindings/phy/qcom,msm8998-qmp-usb3-phy.yaml | 2 ++
>>>>>>>>>>> 1 file changed, 2 insertions(+)
>>>>>>>>>>
>>>>>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
>>>>>>>>>
>>>>>>>>> ... and then I looked at the driver. So un-reviewed. Devices are clearly
>>>>>>>>> compatible. If not, explain what is not compatible.
>>>>>>>>>
>>>>>>>> Talos uses GCC_USB3_PRIM_PHY_AUX_CLK.
>>>>>>>>
>>>>>>>> In Shikra, we are using GCC_USB3_PRIM_PHY_COM_AUX_CLK. We don't have
>>>>>>>> GCC_USB3_PRIM_PHY_AUX_CLK.
>>>>>>>>
>>>>>>>> Hence, I didn't use a fallback compatible.
>>>>>>>
>>>>>>> This still explains nothing. How different clock makes interface for SW
>>>>>>> incompatible exactly?
>>>>>>>
>>>>>> So I went by the naming. AUX vs COM_AUX.
>>>>>
>>>>> The naming does not matter. If the clock is called
>>>>> "no_one_expects_spanish_inquisition", does that make software
>>>>> incompatible? Why would the name itself matter?
>>>>>
>>>>>>
>>>>>> Can I use a fallback compatible and in DT vote for "COM_AUX" clock with
>>>>>> clock-names mentioning "aux" ?
>>>>>
>>>>> I don't know, I asked what is different in software interface.
>>>>>
>>>>
>>>> Hi Krzysztof,
>>>>
>>>> I checked with the hw team here and found out two things.
>>>>
>>>> 1. Shikra is a spinoff of Agatti and its sw interface (clocks used and
>>>> regulators used) is the same as agatti.
>>>>
>>>> 2. I thought we could use qcm2290 as a fallback since the phy register
>>>> init sequence is the same for Talos/Shikra/Agatti. The difference
>>>> between Talos and agatti when checked in the driver was the init load
>>>> settings. I checked with the hw team and they suggested using the init
>>>> load settings which talos was using.
>>>>
>>>> Hence both these compatibles (qcm2290 and qcs615) cannot be used as
>>>> fallback for Shikra.
>>>
>>> Then I do not understand why you are using qcs615_usb3phy_cfg for
>>> Shikra. You say that the initialization is different, but you use
>>> exactly the same initialization. So in a meaning of compatibility
>>> between hardware for Devicetree they are compatible.
>>>
>> Hi Krzysztof,
>>
>> There are 3 things:
>>
>> 1. Clocks used:
>> -> Talos supports AUX Clock since it supports DP over USB.
>> -> Agatti and Shikra use COM_AUX clock since they dont support DP over USB.
>>
>> 2. Phy register Init sequence - same for all 3 targets
>>
>> 3. Regulator init load:
>> -> Different for both Talos and Agatti
>> -> Recommendation is to use Talos regulator load values.
>>
>> SW interface wise, shikra is comaptible with agatti. If we use agatti as
>> fallback, we would end up using the platform data of Agatti where the
>> regulator init load is not suitable for Shikra. Hence not using Agatti
>> as fallback.
>>
>> Coming to driver changes, I used qcs615_cfg because it has required phy
>> register sequence and regulator init load as needed by shikra.
>
> So is it compatible with QCS615? If not, then something is incomplete or
> confusing. The driver uses the same software interface.
>
Sorry for the confusion. The Talos compatible represents the USB/DP PHY
with aux clock input, while Shikra is a USB-only PHY with com_aux input
clock, so the two PHYs are not compatible with each other.
In the Linux driver implementation the match data is currently used only
to affect the init sequence and regulator init load and here Shikra can
reuse the Talos match data structure.
Regards,
Krishna,
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* sashiko review: Re: [PATCH] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context
From: Claudiu Beznea @ 2026-05-15 15:01 UTC (permalink / raw)
To: yoshihiro.shimoda.uh, vkoul, neil.armstrong, geert+renesas,
magnus.damm, prabhakar.mahadev-lad.rj
Cc: linux-renesas-soc, linux-phy, linux-kernel, Claudiu Beznea,
stable, Pavel Machek, Nobuhiro Iwamatsu
In-Reply-To: <20260514111300.2152386-1-claudiu.beznea@kernel.org>
>commit 856d28e68a5bcfa08fa03aa685cb15772197dedc
>Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context
>This commit releases the spin lock before sleeping for 20 ms during
>OTG PHY initialization and introduces completion mechanisms to prevent
>other threads from configuring the hardware concurrently. It also disables
>IRQs while waiting to prevent interrupt handlers from applying settings.
>Closes: https://lore.kernel.org/all/afhkX2Ys2BG1gnqy@duo.ucw.cz
>Closes: https://lore.kernel.org/all/afhkX2Ys2BG1gnqy@duo.ucw.cz
>> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>> index 79e820e2fe55d..5a18bbd87c3b9 100644
>> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>[ ... ]
>> @@ -388,32 +392,68 @@ static bool rcar_gen3_are_all_rphys_power_off(struct
rcar_gen3_chan *ch)
>> return true;
>> }
>>
>> +static int rcar_gen3_phy_wait_otg_init(struct rcar_gen3_chan *channel,
>> + unsigned long *flags)
>> +{
>> + unsigned long timeout = msecs_to_jiffies(25);
>Could this completion timeout be too tight?
I tried to have a bit increased compared with the fsleep() value to avoid just
being to tight but not to big.
>The initializing thread uses fsleep(20000) which guarantees a minimum sleep
>of 20ms. Due to scheduler jitter or timer wheel granularity (especially if
>HZ=100, where 25ms rounds to just 2 or 3 jiffies), the 20ms sleep can easily
>exceed 25ms of wall-clock time, causing this to spuriously return -ETIMEDOUT.
>> + unsigned long ret = 1;
>> +
>> + lockdep_assert_held(&channel->lock);
>[ ... ]
>> @@ -427,7 +467,10 @@ static ssize_t role_store(struct device *dev, struct
device_attribute *attr,
>> const char *buf, size_t count)
>> {
>> struct rcar_gen3_chan *ch = dev_get_drvdata(dev);
>> bool is_b_device;
>> enum phy_mode cur_mode, new_mode;
>> + unsigned long flags;
>> + int ret = -EIO;
>>
>> - guard(spinlock_irqsave)(&ch->lock);
>> + spin_lock_irqsave(&ch->lock, flags);
>>
>> if (!ch->is_otg_channel || !rcar_gen3_is_any_otg_rphy_initialized(ch))
>> - return -EIO;
>> + goto unlock;
>> +
>> + ret = rcar_gen3_phy_wait_otg_init(ch, &flags);
>Does checking the initialized state before waiting for the lock to be
>released introduce a race condition?
>Because rcar_gen3_phy_usb2_init() only sets rphy->initialized = true after
>it finishes its 20ms sleep and reacquires the lock, a concurrent sysfs write
>to role_store() during this 20ms window will see initialized as false and
>immediately return -EIO instead of waiting for initialization to complete.
It is better to have it as suggested here, thus I'll adjust it as proposed.
>> + if (ret)
>> + goto unlock;
>[ ... ]
>> @@ -512,6 +556,11 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void
*_ch)
>> goto rpm_put;
>>
>> scoped_guard(spinlock, &ch->lock) {
>> + if (READ_ONCE(ch->otg_initializing)) {
>> + dev_warn(dev, "%s: Got IRQ while waiting for OTG
init!\n", __func__);
>> + return IRQ_NONE;
>> + }
>Does returning early here leak the PM runtime reference?
Indeed, I missed that, will adjust it in the next version.
>The interrupt handler calls pm_runtime_get_noresume(dev) at the beginning of
>the function, and releases it at the rpm_put label. By using return IRQ_NONE
>inside the scoped_guard, the code bypasses pm_runtime_put_noidle() completely,
>preventing the device from ever entering runtime suspend.
>Additionally, since this interrupt is requested with IRQF_SHARED, could
>another device sharing the line trigger a valid interrupt during the 20ms
>OTG initialization window? If so, this would print a spurious warning to
>the kernel log for a valid shared interrupt event.
That's a valid point. Better would be to just mask the interrupt locally.
>> +
>> status = readl(usb2_base + USB2_OBINTSTA);
>> if (status & ch->phy_data->obint_enable_bits) {
>> dev_vdbg(dev, "%s: %08x\n", __func__, status);
>[ ... ]
>> @@ -550,8 +605,22 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
>> }
>>
>> /* Initialize otg part (only if we initialize a PHY with IRQs). */
>> - if (rphy->int_enable_bits)
>> - rcar_gen3_init_otg(channel);
>> + if (rphy->int_enable_bits && channel->is_otg_channel &&
>> + !rcar_gen3_is_any_otg_rphy_initialized(channel)) {
>> + rcar_gen3_init_otg_phase0(channel);
>> + disable_irq_nosync(channel->irq);
>Could channel->irq be 0 here?
In theory, AFAIK, this code should not be executed when the interrupt is not
present as the OTG PHY needs the interrupt.
>In rcar_gen3_phy_usb2_probe(), platform_get_irq_optional() is used. If it
>returns -ENXIO, channel->irq remains 0. Since rphy->int_enable_bits is a
>statically defined non-zero value for OTG PHYs, this would pass 0 to
>disable_irq_nosync(), which manipulates critical system interrupts.
>Also, does calling disable_irq_nosync() on a shared interrupt line starve
>other devices?
>Since the interrupt is requested with IRQF_SHARED, masking it at the
>interrupt controller level means all other unrelated devices on the same
>physical line will have their interrupts disabled for the full 20ms
>duration. Should shared interrupts be masked locally at the device level
>instead?
That would be better, indeed.
>> + reinit_completion(&channel->otg_init_done);
>> + WRITE_ONCE(channel->otg_initializing, true);
>> + spin_unlock_irqrestore(&channel->lock, flags);
>[ ... ]
>> @@ -598,6 +678,7 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
>> struct rcar_gen3_phy *rphy = phy_get_drvdata(p);
>> struct rcar_gen3_chan *channel = rphy->ch;
>> void __iomem *usb2_base = channel->base;
>> + unsigned long flags;
>> u32 val;
>> int ret = 0;
>>
>> @@ -607,11 +688,15 @@ static int rcar_gen3_phy_usb2_power_on(struct phy *p)
>> return ret;
>> }
>>
>> - guard(spinlock_irqsave)(&channel->lock);
>> + spin_lock_irqsave(&channel->lock, flags);
>>
>> if (!rcar_gen3_are_all_rphys_power_off(channel))
>> goto out;
>>
>> + ret = rcar_gen3_phy_wait_otg_init(channel, &flags);
>> + if (ret)
>> + goto unlock;
>> +
>> val = readl(usb2_base + USB2_USBCTR);
>> val |= USB2_USBCTR_PLL_RST;
>Does dropping the lock in rcar_gen3_phy_wait_otg_init() introduce a race
>condition with the !rcar_gen3_are_all_rphys_power_off() check?
>If two threads concurrently power on different PHYs, both will evaluate the
>condition as false because neither has reached rphy->powered = true at the
>end of the function. Both threads might then sleep in
>rcar_gen3_phy_wait_otg_init().
>Upon waking and reacquiring the lock, both will unconditionally apply
>USB2_USBCTR_PLL_RST, which could catastrophically disrupt the PHY that was
>just initialized by the first thread.
>Should the power off condition be re-evaluated after the lock is reacquired?
Yes, rcar_gen3_phy_wait_otg_init() should be called first.
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context
From: Claudiu Beznea @ 2026-05-15 14:37 UTC (permalink / raw)
To: David Laight, Pavel Machek
Cc: yoshihiro.shimoda.uh, vkoul, neil.armstrong, geert+renesas,
magnus.damm, prabhakar.mahadev-lad.rj, linux-renesas-soc,
linux-phy, linux-kernel, Claudiu Beznea, stable,
Nobuhiro Iwamatsu
In-Reply-To: <20260515104749.24135f22@pumpkin>
Hi, David,
On 5/15/26 12:47, David Laight wrote:
> On Thu, 14 May 2026 23:18:44 +0200
> Pavel Machek <pavel@nabladev.com> wrote:
>
>> Hi!
>>
>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> The OTG PHY initialization sequence needs to wait for 20 ms at a specific
>>> step, as described in commit 72c0339c115b ("phy: renesas:
>>> rcar-gen3-usb2: follow the hardware manual procedure").
>>>
>>> Commit 55a387ebb921 ("phy: renesas: rcar-gen3-usb2: Lock around hardware
>>> registers and driver data") tried to address various problems in the
>>> rcar-gen3-usb2 driver and converted the mutex protecting HW register
>>> accesses to a spin lock, leaving, however, a long delay in the critical
>>> section protected by the spin lock. This may become a problem,
>>> especially on RT kernels.
>>>
>>> To address this, release the spin lock before sleeping for 20 ms as
>>> required by the HW manual and reacquire it afterwards. To avoid other
>>> threads entering the critical section and configuring the HW while the
>>> software is waiting for the OTG initialization to complete, introduce the
>>> otg_initializing variable alongside the otg_init_done completion. Any
>>> other thread trying to configure the HW while the OTG PHY initialization
>>> is in progress waits for the completion instead of immediately returning
>>> errors to PHY users. The IRQs were also disabled while waiting for the OTG
>>> PHY initialization to complete, as the interrupt handler may also apply HW
>>> settings.
>>
>> Just... there has to be a better way.
>>
>>> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>>> +static int rcar_gen3_phy_wait_otg_init(struct rcar_gen3_chan *channel,
>>> + unsigned long *flags)
>>> +{
>>> + unsigned long timeout = msecs_to_jiffies(25);
>>> + unsigned long ret = 1;
>>> +
>>> + lockdep_assert_held(&channel->lock);
>>> +
>>> + /*
>>> + * The OTG can be initialized only once and needs to release the lock
>>> + * and wait for 20 ms due to hardware constraints. Wait for the OTG PHY
>>> + * initialization to complete if another PHY executes configuration
>>> + * code while the OTG PHY is waiting. This avoids returning failures to
>>> + * PHY users.
>>> + */
>>> + if (READ_ONCE(channel->otg_initializing)) {
>>> + spin_unlock_irqrestore(&channel->lock, *flags);
>>
>> This is not nice, passing flags between functions like this is a red flag.
>
> It would be better to just inline the code.
I can do that, I tried to avoid it.
> And I'd guess you need to redo the initial tests after re-acquiring the lock?
Could you please let me know what do you mean by "initial tests"
> Or even need to do a state change/reference count before releasing the
> lock to stop other threads 'doing anything nasty'.
I'm not sure I understand this. I'll explain how the patch works:
The HW provides a single OTG PHY. As the HW has a single OTG PHY and the start
of the OTG PHY initialization is done under spinlock, at any moment, a single
thread could set the otg_initializing. That would be the thread configuring the
OTG PHY. And once the OTG PHY was set there is no way it will run again (due to
rphy->initialized). Unless the struct phy_ops::exit() is called for the OTG PHY.
With the solution in this patch, once a thread sets the otg_initializing, all
the other threads that will want to set HW registers should wait due to the
completion mechanism.
If there is any thread that executes wait_for_completion() while:
- otg_initializing is set and
- the OTG configuration thread released the spin lock to execute the fsleep()
if the wait_for_completion times out, the PHY code returns error to the caller
so the thread calling into the PHY driver will not continue into the PHY driver.
If instead there is no timeout, the waiting thread will have to re-acquire the
spin lock before executing any HW settings.
If there is no timeout, the code that setup the OTG PHY have already been
running rphy->initialized = true for the OTG PHY, under spinlock, and no other
thread will enter the above OTG initialization section:
/* Initialize otg part (only if we initialize a PHY with IRQs). */
if (rphy->int_enable_bits && channel->is_otg_channel &&
!rcar_gen3_is_any_otg_rphy_initialized(channel)) {
rcar_gen3_init_otg_phase0(channel);
disable_irq_nosync(channel->irq);
reinit_completion(&channel->otg_init_done);
WRITE_ONCE(channel->otg_initializing, true);
spin_unlock_irqrestore(&channel->lock, flags);
fsleep(20000);
spin_lock_irqsave(&channel->lock, flags);
WRITE_ONCE(channel->otg_initializing, false);
complete_all(&channel->otg_init_done);
enable_irq(channel->irq);
rcar_gen3_init_otg_phase1(channel);
}
Thank you,
Claudiu
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v2 phy-next 2/2] phy: ti: add PHY driver for TI DS125DF111 Dual-Channel Retimer
From: Ioana Ciornei @ 2026-05-15 14:36 UTC (permalink / raw)
To: sashiko-reviews
Cc: conor+dt, krzk+dt, devicetree, linux-phy, olteanv, neil.armstrong,
vkoul, robh
In-Reply-To: <20260515112252.E684CC2BCB0@smtp.kernel.org>
On Fri, May 15, 2026 at 11:22:52AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Missing delay or polling after setting a self-clearing hardware reset bit.
> --
>
> commit 59a5ca7411308a81c146d660b8c04e0be8fe8fbb
> Author: Ioana Ciornei <ioana.ciornei@nxp.com>
>
> phy: ti: add PHY driver for TI DS125DF111 Dual-Channel Retimer
>
> This commit adds a generic PHY driver for the TI DS125DF111 Multi-Protocol
> Dual-Channel Retimer. It supports 10G and 1G link speeds, modeling each of
> the two channels as a separate PHY device.
>
> > diff --git a/drivers/phy/ti/phy-ds125df111.c b/drivers/phy/ti/phy-ds125df111.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/drivers/phy/ti/phy-ds125df111.c
>
> [ ... ]
>
> > +static int ds125df111_configure(struct phy *phy,
> > + const struct ds125df111_config *cfg)
> > +{
>
> [ ... ]
>
> > + /* Reset Channel Registers */
> > + err = i2c_smbus_read_byte_data(i2c, DS125DF111_CH_CTRL);
> > + if (err < 0) {
> > + dev_err(dev, "Error resetting channel configuration\n");
> > + goto out;
> > + }
> > + val = (u8)err;
> > + val |= DS125DF111_CH_CTRL_RESET;
> > + err = i2c_smbus_write_byte_data(i2c, DS125DF111_CH_CTRL, val);
> > + if (err < 0) {
> > + dev_err(dev, "Error resetting channel configuration\n");
> > + goto out;
> > + }
> > +
> > + /* Program the VCO group frequencies */
> > + for (i = 0; i < DS125DF111_NUM_VCO_GROUP_REG; i++) {
> > + err = i2c_smbus_write_byte_data(i2c,
> > + DS125DF111_VCO_GROUP_BASE + i,
> > + cfg->vco_group[i]);
>
> Is a delay or polling loop needed after triggering the channel reset?
>
> The DS125DF111_CH_CTRL_RESET bit is defined as self-clearing, but the code
> immediately proceeds to program the VCO group frequencies without checking if
> the reset has finished.
>
> Since hardware resets usually take a non-zero amount of time, writing to the
> registers right away could result in the device NACKing the transactions or
> silently dropping the new configuration.
>
The datasheet does not specify a maximum timeout for channel reset. I
will try to use read_poll_timeout() to wait for the DS125DF111_CH_CTRL
bit to be cleared.
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH] phy: renesas: rcar-gen3-usb2: Avoid long delay in atomic context
From: Claudiu Beznea @ 2026-05-15 14:14 UTC (permalink / raw)
To: Pavel Machek
Cc: yoshihiro.shimoda.uh, vkoul, neil.armstrong, geert+renesas,
magnus.damm, prabhakar.mahadev-lad.rj, linux-renesas-soc,
linux-phy, linux-kernel, Claudiu Beznea, stable,
Nobuhiro Iwamatsu
In-Reply-To: <agY8NAyCcHkhBvBv@duo.ucw.cz>
Hi, Pavel,
On 5/15/26 00:18, Pavel Machek wrote:
> Hi!
>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The OTG PHY initialization sequence needs to wait for 20 ms at a specific
>> step, as described in commit 72c0339c115b ("phy: renesas:
>> rcar-gen3-usb2: follow the hardware manual procedure").
>>
>> Commit 55a387ebb921 ("phy: renesas: rcar-gen3-usb2: Lock around hardware
>> registers and driver data") tried to address various problems in the
>> rcar-gen3-usb2 driver and converted the mutex protecting HW register
>> accesses to a spin lock, leaving, however, a long delay in the critical
>> section protected by the spin lock. This may become a problem,
>> especially on RT kernels.
>>
>> To address this, release the spin lock before sleeping for 20 ms as
>> required by the HW manual and reacquire it afterwards. To avoid other
>> threads entering the critical section and configuring the HW while the
>> software is waiting for the OTG initialization to complete, introduce the
>> otg_initializing variable alongside the otg_init_done completion. Any
>> other thread trying to configure the HW while the OTG PHY initialization
>> is in progress waits for the completion instead of immediately returning
>> errors to PHY users. The IRQs were also disabled while waiting for the OTG
>> PHY initialization to complete, as the interrupt handler may also apply HW
>> settings.
>
> Just... there has to be a better way.
>
>> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>> +static int rcar_gen3_phy_wait_otg_init(struct rcar_gen3_chan *channel,
>> + unsigned long *flags)
>> +{
>> + unsigned long timeout = msecs_to_jiffies(25);
>> + unsigned long ret = 1;
>> +
>> + lockdep_assert_held(&channel->lock);
>> +
>> + /*
>> + * The OTG can be initialized only once and needs to release the lock
>> + * and wait for 20 ms due to hardware constraints. Wait for the OTG PHY
>> + * initialization to complete if another PHY executes configuration
>> + * code while the OTG PHY is waiting. This avoids returning failures to
>> + * PHY users.
>> + */
>> + if (READ_ONCE(channel->otg_initializing)) {
>> + spin_unlock_irqrestore(&channel->lock, *flags);
>
> This is not nice, passing flags between functions like this is a red flag.
>
> You are only accessing otg_initializing under the spinlock. That means
> that READ_ONCE is reduntant.
>
> But AFAICT spinlock is only held over this function to protect
> channel->otg_initializing access. I suspect correct answer here is
> getting rid of spinlock over this function, and using
> test_bit(BIT_INITIALIZING, ...) or something similar.
If I understand correctly your point here, I don't think making the
otg_initializing atomic (or using test_bit()) and moving it out of the spin lock
works for all the cases. Suppose the following:
thread1: thread2:
0: sleep spin_lock_irqsave();
1: still sleep otg_initalizing = 1;
2: check otg_initializing // ...
3: spin_lock_irqsave(); // ...
4: spin_unlock_irqsave();
5: fsleep(20000);
In this way, thread1 will get access to the HW registers once instruction at
line 4 will be executed and be able to configure other HW registers when it
should wait for the fsleep() to finish.
The point with the solution provided in this patch was to not allow any other
thread to configure the HW while the fsleep() is executed.
Thank you,
Claudiu
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v2 phy-next 2/2] phy: ti: add PHY driver for TI DS125DF111 Dual-Channel Retimer
From: Ioana Ciornei @ 2026-05-15 13:44 UTC (permalink / raw)
To: Vladimir Oltean
Cc: vkoul, neil.armstrong, robh, krzk+dt, conor+dt, johan, linux-phy,
devicetree, linux-kernel
In-Reply-To: <20260515122418.edzemzv5gnronihb@skbuf>
On Fri, May 15, 2026 at 03:24:18PM +0300, Vladimir Oltean wrote:
> On Fri, May 15, 2026 at 02:01:45PM +0300, Ioana Ciornei wrote:
> > Add a generic PHY driver for the TI DS125DF111 Multi-Protocol
> > Dual-Channel Retimer. The driver currently supports only 10G and 1G link
> > speeds but it can easily extended to also cover other usecases.
> >
> > Since the available datasheet (https://www.ti.com/lit/gpn/DS125DF111)
> > does not name the registers, the name for the macros were determined by
> > their usage pattern.
> >
> > A PHY device is created for each of the two channels present on the
> > retimer. This allows for independent configuration of the two channels.
> > This capability is especially important on retimers which have more than
> > 2 channels that can be, depending on the board design, connected in
> > multiple different ways to the SerDes lanes.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---
> > Changes in v2:
> > - Explicitly include all the needed headers
> > - Change ds125df111_xlate() so that it returns an error if args_count is
> > not exactly 1
> > - Add a MAINTAINERS entry
> > ---
> > MAINTAINERS | 7 +
> > drivers/phy/ti/Kconfig | 10 ++
> > drivers/phy/ti/Makefile | 1 +
> > drivers/phy/ti/phy-ds125df111.c | 252 ++++++++++++++++++++++++++++++++
> > 4 files changed, 270 insertions(+)
> > create mode 100644 drivers/phy/ti/phy-ds125df111.c
> >
(...)
> > +static int ds125df111_configure(struct phy *phy,
> > + const struct ds125df111_config *cfg)
> > +{
> > + struct ds125df111_ch *ch = phy_get_drvdata(phy);
> > + struct ds125df111_priv *priv = ch->priv;
> > + struct i2c_client *i2c = priv->client;
> > + struct device *dev = &phy->dev;
> > + u8 val;
> > + int err, i;
>
> Not mandatory, but if the rest of the file uses reverse Christmas tree
> variable ordering, could you stick to that here as well?
Ok, sure.
>
> > +
> > + mutex_lock(&priv->mutex);
> > +
> > + /* Make sure that any subsequent read/write operation will be directed
> > + * only to the registers of the selected channel
> > + */
> > + err = i2c_smbus_read_byte_data(i2c, DS125DF111_CH_SELECT);
> > + if (err < 0) {
> > + dev_err(dev, "Unable to select channel\n");
>
> Here and everywhere else: could you please print a symbolic description
> of the error? %pe, ERR_PTR(err).
Nice, I didn't know about the %pe.
>
> > + goto out;
> > + }
> > + val = (u8)err;
> > + val &= ~DS125DF111_CH_SELECT_TARGET_MASK;
> > + val |= DS125DF111_CH_SELECT_EN | ch->idx;
> > + err = i2c_smbus_write_byte_data(i2c, DS125DF111_CH_SELECT, val);
> > + if (err < 0) {
> > + dev_err(dev, "Unable to select channel\n");
> > + goto out;
> > + }
> > +
> > + /* Reset Channel Registers */
> > + err = i2c_smbus_read_byte_data(i2c, DS125DF111_CH_CTRL);
> > + if (err < 0) {
> > + dev_err(dev, "Error resetting channel configuration\n");
> > + goto out;
> > + }
> > + val = (u8)err;
> > + val |= DS125DF111_CH_CTRL_RESET;
> > + err = i2c_smbus_write_byte_data(i2c, DS125DF111_CH_CTRL, val);
> > + if (err < 0) {
> > + dev_err(dev, "Error resetting channel configuration\n");
> > + goto out;
> > + }
>
> Did you consider simplifying this function using a ds125df111_rmw() helper?
> All configuration accesses except the VCO group frequencies are
> read-modify-write.
>
I will look into a _rmw helper and see if it helps.
> > +
> > + /* Program the VCO group frequencies */
> > + for (i = 0; i < DS125DF111_NUM_VCO_GROUP_REG; i++) {
> > + err = i2c_smbus_write_byte_data(i2c,
> > + DS125DF111_VCO_GROUP_BASE + i,
> > + cfg->vco_group[i]);
> > + if (err < 0) {
> > + dev_err(dev, "Error programming VCO group frequencies\n");
> > + goto out;
> > + }
> > + }
> > +
> > + /* Set the Divide Ratios for the VCO Groups*/
>
> Space between Groups and */
> Also, Divide Ratios, Groups, Channel Registers are not proper nouns,
> they don't need to be capitalized.
Ok, will fix.
>
> > + err = i2c_smbus_read_byte_data(i2c, DS125DF111_RATIOS);
> > + if (err < 0) {
> > + dev_err(dev, "Error programming the divide ratios\n");
> > + goto out;
> > + }
> > + val = (u8)err;
> > + val &= ~(DS125DF111_RATIOS_RATE_MASK | DS125DF111_RATIOS_SUBRATE_MASK);
> > + val |= FIELD_PREP(DS125DF111_RATIOS_RATE_MASK, cfg->rate) |
> > + FIELD_PREP(DS125DF111_RATIOS_SUBRATE_MASK, cfg->subrate);
> > + err = i2c_smbus_write_byte_data(i2c, DS125DF111_RATIOS, val);
> > + if (err < 0) {
> > + dev_err(dev, "Error programming the divide ratios\n");
> > + goto out;
> > + }
> > +
> > + mutex_unlock(&priv->mutex);
> > +
> > + return 0;
> > +
> > +out:
> > + mutex_unlock(&priv->mutex);
> > +
> > + return err;
>
> You don't need a separate code path for the 'out' label, it can be
> common with the normal exit path (err will be 0).
Ok.
>
> > +}
> > +
> > +static int ds125df111_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> > +{
> > + const struct ds125df111_config *cfg;
> > +
> > + if (mode != PHY_MODE_ETHERNET)
> > + return -EOPNOTSUPP;
>
> Please use a different error code like -EINVAL. Let -EOPNOTSUPP mean
> that the function is not implemented (when calling phy_set_mode_ext()).
>
> > +
> > + switch (submode) {
> > + case PHY_INTERFACE_MODE_10GBASER:
> > + cfg = &ds125df111_cfg[FREQ_10G];
> > + break;
> > + case PHY_INTERFACE_MODE_1000BASEX:
> > + case PHY_INTERFACE_MODE_SGMII:
> > + cfg = &ds125df111_cfg[FREQ_1G];
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
>
> Same here.
Will change in both instances.
>
> > + }
> > +
> > + return ds125df111_configure(phy, cfg);
> > +}
> > +
> > +static const struct phy_ops ds125df111_ops = {
> > + .set_mode = ds125df111_set_mode,
>
> Can you please implement .validate() as well? It will be made mandatory
> in the future for those who implement .set_mode().
>
Sure. Will implement the .validate() callback in v3.
(...)
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static const struct of_device_id ds125df111_dt_ids[] =
> > + { .compatible = "ti,ds125df111", },
> > + {},
>
> Unnecessary comma after sentinel entry.
I will remove it.
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v2 phy-next 2/2] phy: ti: add PHY driver for TI DS125DF111 Dual-Channel Retimer
From: Vladimir Oltean @ 2026-05-15 12:24 UTC (permalink / raw)
To: Ioana Ciornei
Cc: vkoul, neil.armstrong, robh, krzk+dt, conor+dt, johan, linux-phy,
devicetree, linux-kernel
In-Reply-To: <20260515110145.1925579-3-ioana.ciornei@nxp.com>
On Fri, May 15, 2026 at 02:01:45PM +0300, Ioana Ciornei wrote:
> Add a generic PHY driver for the TI DS125DF111 Multi-Protocol
> Dual-Channel Retimer. The driver currently supports only 10G and 1G link
> speeds but it can easily extended to also cover other usecases.
>
> Since the available datasheet (https://www.ti.com/lit/gpn/DS125DF111)
> does not name the registers, the name for the macros were determined by
> their usage pattern.
>
> A PHY device is created for each of the two channels present on the
> retimer. This allows for independent configuration of the two channels.
> This capability is especially important on retimers which have more than
> 2 channels that can be, depending on the board design, connected in
> multiple different ways to the SerDes lanes.
>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
> Changes in v2:
> - Explicitly include all the needed headers
> - Change ds125df111_xlate() so that it returns an error if args_count is
> not exactly 1
> - Add a MAINTAINERS entry
> ---
> MAINTAINERS | 7 +
> drivers/phy/ti/Kconfig | 10 ++
> drivers/phy/ti/Makefile | 1 +
> drivers/phy/ti/phy-ds125df111.c | 252 ++++++++++++++++++++++++++++++++
> 4 files changed, 270 insertions(+)
> create mode 100644 drivers/phy/ti/phy-ds125df111.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f877e5aaf2c7..58f410b666e7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26781,6 +26781,13 @@ T: git git://linuxtv.org/mhadli/v4l-dvb-davinci_devices.git
> F: drivers/media/platform/ti/davinci/
> F: include/media/davinci/
>
> +TI DS125DF111 RETIMER PHY DRIVER
> +M: Ioana Ciornei <ioana.ciornei@nxp.com>
> +L: linux-phy@lists.infradead.org (moderated for non-subscribers)
> +S: Maintained
> +F: Documentation/devicetree/bindings/phy/ti,ds125df111.yaml
> +F: drivers/phy/ti/phy-ds125df111.c
> +
> TI ENHANCED CAPTURE (eCAP) DRIVER
> M: Vignesh Raghavendra <vigneshr@ti.com>
> R: Julien Panis <jpanis@baylibre.com>
> diff --git a/drivers/phy/ti/Kconfig b/drivers/phy/ti/Kconfig
> index b40f28019131..475e80fcd52d 100644
> --- a/drivers/phy/ti/Kconfig
> +++ b/drivers/phy/ti/Kconfig
> @@ -111,3 +111,13 @@ config PHY_TI_GMII_SEL
> help
> This driver supports configuring of the TI CPSW Port mode depending on
> the Ethernet PHY connected to the CPSW Port.
> +
> +config PHY_TI_DS125DF111
> + tristate "DS125DF111 2-Channel Retimer Driver"
> + depends on OF && I2C
> + select GENERIC_PHY
> + help
> + Enable this to add support for configuration and runtime management
> + of the TI DS125DF111 Multi-Protocol 2-Channel Retimer.
> + The retimer is modeled as a Generic PHY and supports both 10G and 1G
> + link speeds.
> diff --git a/drivers/phy/ti/Makefile b/drivers/phy/ti/Makefile
> index dcba2571c9bd..e68445ddd848 100644
> --- a/drivers/phy/ti/Makefile
> +++ b/drivers/phy/ti/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o
> obj-$(CONFIG_PHY_AM654_SERDES) += phy-am654-serdes.o
> obj-$(CONFIG_PHY_TI_GMII_SEL) += phy-gmii-sel.o
> obj-$(CONFIG_PHY_J721E_WIZ) += phy-j721e-wiz.o
> +obj-$(CONFIG_PHY_TI_DS125DF111) += phy-ds125df111.o
> diff --git a/drivers/phy/ti/phy-ds125df111.c b/drivers/phy/ti/phy-ds125df111.c
> new file mode 100644
> index 000000000000..084a94e655c2
> --- /dev/null
> +++ b/drivers/phy/ti/phy-ds125df111.c
> @@ -0,0 +1,252 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright 2026 NXP */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/phy.h>
> +#include <linux/phy/phy.h>
> +#include <linux/slab.h>
> +
> +#define DS125DF111_NUM_CH 2
> +#define DS125DF111_NUM_VCO_GROUP_REG 5
> +
> +#define DS125DF111_CH_SELECT 0xff
> +#define DS125DF111_CH_SELECT_TARGET_MASK GENMASK(3, 0)
> +#define DS125DF111_CH_SELECT_EN BIT(2)
> +
> +#define DS125DF111_CH_CTRL 0x00
> +#define DS125DF111_CH_CTRL_RESET BIT(2) /* self clearing */
> +
> +#define DS125DF111_VCO_GROUP_BASE 0x60
> +
> +#define DS125DF111_RATIOS 0x2F
> +#define DS125DF111_RATIOS_RATE_MASK GENMASK(7, 6)
> +#define DS125DF111_RATIOS_SUBRATE_MASK GENMASK(5, 4)
> +
> +struct ds125df111_ch {
> + struct phy *phy;
> + struct ds125df111_priv *priv;
> + int idx;
> +};
> +
> +struct ds125df111_priv {
> + struct ds125df111_ch ch[DS125DF111_NUM_CH];
> + struct i2c_client *client;
> + struct mutex mutex; /* protects access to shared registers */
> +};
> +
> +enum ds125df111_mode {
> + FREQ_1G,
> + FREQ_10G,
> +};
> +
> +static const struct ds125df111_config {
> + u8 vco_group[DS125DF111_NUM_VCO_GROUP_REG];
> + u8 rate;
> + u8 subrate;
> +} ds125df111_cfg[] = {
> + [FREQ_1G] = {
> + /* VCO group #0 = 10GHz, VCO group #1 = 10GHz */
> + .vco_group = {0x00, 0xB2, 0x00, 0xB2, 0xCC},
> + /* By using the following combination of rate and subrate we
> + * select divide ratios of 1, 2, 4, 8 on both groups
> + */
> + .rate = 0x1,
> + .subrate = 0x2,
> + },
> +
> + [FREQ_10G] = {
> + /* VCO group #0 = 10.3125GHz, VCO group #1 = 10.3125GHz */
> + .vco_group = {0x90, 0xB3, 0x90, 0xB3, 0xCD},
> + /* By using the following combination of rate and subrate we
> + * select divide ratios of 1 on both groups
> + */
> + .rate = 0x1,
> + .subrate = 0x3,
> + },
> +};
> +
> +static int ds125df111_configure(struct phy *phy,
> + const struct ds125df111_config *cfg)
> +{
> + struct ds125df111_ch *ch = phy_get_drvdata(phy);
> + struct ds125df111_priv *priv = ch->priv;
> + struct i2c_client *i2c = priv->client;
> + struct device *dev = &phy->dev;
> + u8 val;
> + int err, i;
Not mandatory, but if the rest of the file uses reverse Christmas tree
variable ordering, could you stick to that here as well?
> +
> + mutex_lock(&priv->mutex);
> +
> + /* Make sure that any subsequent read/write operation will be directed
> + * only to the registers of the selected channel
> + */
> + err = i2c_smbus_read_byte_data(i2c, DS125DF111_CH_SELECT);
> + if (err < 0) {
> + dev_err(dev, "Unable to select channel\n");
Here and everywhere else: could you please print a symbolic description
of the error? %pe, ERR_PTR(err).
> + goto out;
> + }
> + val = (u8)err;
> + val &= ~DS125DF111_CH_SELECT_TARGET_MASK;
> + val |= DS125DF111_CH_SELECT_EN | ch->idx;
> + err = i2c_smbus_write_byte_data(i2c, DS125DF111_CH_SELECT, val);
> + if (err < 0) {
> + dev_err(dev, "Unable to select channel\n");
> + goto out;
> + }
> +
> + /* Reset Channel Registers */
> + err = i2c_smbus_read_byte_data(i2c, DS125DF111_CH_CTRL);
> + if (err < 0) {
> + dev_err(dev, "Error resetting channel configuration\n");
> + goto out;
> + }
> + val = (u8)err;
> + val |= DS125DF111_CH_CTRL_RESET;
> + err = i2c_smbus_write_byte_data(i2c, DS125DF111_CH_CTRL, val);
> + if (err < 0) {
> + dev_err(dev, "Error resetting channel configuration\n");
> + goto out;
> + }
Did you consider simplifying this function using a ds125df111_rmw() helper?
All configuration accesses except the VCO group frequencies are
read-modify-write.
> +
> + /* Program the VCO group frequencies */
> + for (i = 0; i < DS125DF111_NUM_VCO_GROUP_REG; i++) {
> + err = i2c_smbus_write_byte_data(i2c,
> + DS125DF111_VCO_GROUP_BASE + i,
> + cfg->vco_group[i]);
> + if (err < 0) {
> + dev_err(dev, "Error programming VCO group frequencies\n");
> + goto out;
> + }
> + }
> +
> + /* Set the Divide Ratios for the VCO Groups*/
Space between Groups and */
Also, Divide Ratios, Groups, Channel Registers are not proper nouns,
they don't need to be capitalized.
> + err = i2c_smbus_read_byte_data(i2c, DS125DF111_RATIOS);
> + if (err < 0) {
> + dev_err(dev, "Error programming the divide ratios\n");
> + goto out;
> + }
> + val = (u8)err;
> + val &= ~(DS125DF111_RATIOS_RATE_MASK | DS125DF111_RATIOS_SUBRATE_MASK);
> + val |= FIELD_PREP(DS125DF111_RATIOS_RATE_MASK, cfg->rate) |
> + FIELD_PREP(DS125DF111_RATIOS_SUBRATE_MASK, cfg->subrate);
> + err = i2c_smbus_write_byte_data(i2c, DS125DF111_RATIOS, val);
> + if (err < 0) {
> + dev_err(dev, "Error programming the divide ratios\n");
> + goto out;
> + }
> +
> + mutex_unlock(&priv->mutex);
> +
> + return 0;
> +
> +out:
> + mutex_unlock(&priv->mutex);
> +
> + return err;
You don't need a separate code path for the 'out' label, it can be
common with the normal exit path (err will be 0).
> +}
> +
> +static int ds125df111_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> +{
> + const struct ds125df111_config *cfg;
> +
> + if (mode != PHY_MODE_ETHERNET)
> + return -EOPNOTSUPP;
Please use a different error code like -EINVAL. Let -EOPNOTSUPP mean
that the function is not implemented (when calling phy_set_mode_ext()).
> +
> + switch (submode) {
> + case PHY_INTERFACE_MODE_10GBASER:
> + cfg = &ds125df111_cfg[FREQ_10G];
> + break;
> + case PHY_INTERFACE_MODE_1000BASEX:
> + case PHY_INTERFACE_MODE_SGMII:
> + cfg = &ds125df111_cfg[FREQ_1G];
> + break;
> + default:
> + return -EOPNOTSUPP;
Same here.
> + }
> +
> + return ds125df111_configure(phy, cfg);
> +}
> +
> +static const struct phy_ops ds125df111_ops = {
> + .set_mode = ds125df111_set_mode,
Can you please implement .validate() as well? It will be made mandatory
in the future for those who implement .set_mode().
> + .owner = THIS_MODULE,
> +};
> +
> +static struct phy *ds125df111_xlate(struct device *dev,
> + const struct of_phandle_args *args)
> +{
> + struct ds125df111_priv *priv = dev_get_drvdata(dev);
> + u32 idx;
> +
> + if (args->args_count != 1)
> + return ERR_PTR(-EINVAL);
> +
> + idx = args->args[0];
> + if (idx >= DS125DF111_NUM_CH) {
> + dev_err(dev, "Maximum number of channels is %d\n",
> + DS125DF111_NUM_CH);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return priv->ch[idx].phy;
> +}
> +
> +static int ds125df111_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct phy_provider *provider;
> + struct ds125df111_priv *priv;
> + int i, err;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> + priv->client = client;
> + err = devm_mutex_init(dev, &priv->mutex);
> + if (err)
> + return err;
> +
> + i2c_set_clientdata(client, priv);
> +
> + for (i = 0; i < DS125DF111_NUM_CH; i++) {
> + struct ds125df111_ch *ch = &priv->ch[i];
> + struct phy *phy;
> +
> + phy = devm_phy_create(dev, NULL, &ds125df111_ops);
> + if (IS_ERR(phy))
> + return PTR_ERR(phy);
> +
> + ch->idx = i;
> + ch->priv = priv;
> + ch->phy = phy;
> +
> + phy_set_drvdata(phy, ch);
> + }
> +
> + provider = devm_of_phy_provider_register(dev, ds125df111_xlate);
> +
> + return PTR_ERR_OR_ZERO(provider);
> +}
> +
> +static const struct of_device_id ds125df111_dt_ids[] = {
> + { .compatible = "ti,ds125df111", },
> + {},
Unnecessary comma after sentinel entry.
> +};
> +MODULE_DEVICE_TABLE(of, ds125df111_dt_ids);
> +
> +static struct i2c_driver ds125df111_driver = {
> + .driver = {
> + .name = "ds125df111",
> + .of_match_table = ds125df111_dt_ids,
> + },
> + .probe = ds125df111_probe,
> +};
> +module_i2c_driver(ds125df111_driver);
> +
> +MODULE_AUTHOR("Ioana Ciornei <ioana.ciornei@nxp.com>");
> +MODULE_DESCRIPTION("TI DS125DF111 Retimer driver");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>
>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH] net: phy: sfp: probe for RollBall I2C-to-MDIO bridge before assuming MDIO_I2C_ROLLBALL
From: Petr Wozniak @ 2026-05-15 12:15 UTC (permalink / raw)
To: netdev; +Cc: bjorn, andrew, linux-phy, Petr Wozniak
In-Reply-To: <20260515121108.17792-1-petr.wozniak@gmail.com>
Please disregard this patch — it is a duplicate of:
https://lore.kernel.org/netdev/20260515115527.17241-1-petr.wozniak@gmail.com/
The original was sent to linux-netdev@vger.kernel.org and I received a
bounce notification. I incorrectly assumed delivery had failed and
resent. The original did reach the list.
Sorry for the noise.
Petr
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH] net: phy: sfp: probe for RollBall I2C-to-MDIO bridge before assuming MDIO_I2C_ROLLBALL
From: Petr Wozniak @ 2026-05-15 12:11 UTC (permalink / raw)
To: netdev; +Cc: bjorn, andrew, linux-phy, Petr Wozniak
The "OEM"/"SFP-10G-T" quirk entry in sfp_fixup_rollball_cc()
unconditionally forces MDIO_I2C_ROLLBALL for all modules matching that
vendor/part-number combination. This works for modules that genuinely
implement a RollBall I2C-to-MDIO bridge, but silently breaks modules
that share the same EEPROM strings without having such a bridge.
The Realtek RTL8261BE-CG is one such module: a pure copper 10G SFP+
media converter with no I2C-to-MDIO bridge. Its EEPROM reports
vendor="OEM", part="SFP-10G-T", and -- critically -- Vendor OUI
00:00:00, making OUI-based differentiation impossible. With
MDIO_I2C_ROLLBALL the kernel stalls waiting for a PHY that never
appears:
sfp sfp2: probing phy device through the [MDIO_I2C_ROLLBALL] protocol
sfp sfp2: no PHY detected, 24 tries left
sfp sfp2: no PHY detected, 23 tries left
...
Fix this by probing for the RollBall bridge before committing to a
protocol. The probe sends the RollBall unlock password (four 0xFF bytes
to A2h:VSL+3), switches to MDIO page 3, issues CMD_READ, and polls for
CMD_DONE. A genuine RollBall bridge responds with CMD_DONE within ~70
ms. A module without a bridge never asserts it; the probe times out
after 200 ms (10 x 20 ms).
On probe success the existing MDIO_I2C_ROLLBALL + extended_cc path is
taken -- no behaviour change for real RollBall modules. On timeout,
MDIO_I2C_NONE is selected, telling the MAC to derive link parameters
directly from the EEPROM-declared interface type (10gbase-r) without
attempting PHY register access.
The probe adds at most 200 ms at link-up time for the no-bridge case,
and ~70 ms for real RollBall modules. This is a one-shot cost, not on
the data path.
Also add a separate quirk entry for the industrial variant "SFP-10G-T-I"
which uses the same RTL8261BE silicon.
Tested on BPI-R4 (MediaTek MT7988A, Linux 6.12):
- RTL8261BE negative case (no bridge, probe timeout -> MDIO_I2C_NONE):
link up 10Gbps, iperf3 9.34 Gbit/s (93% line rate) [OK]
- RollBall positive case (CMD_DONE -> MDIO_I2C_ROLLBALL): logic mirrors
the unlock sequence already in mdio-i2c.c; not yet verified on
physical RollBall hardware due to lack of a suitable test module.
Signed-off-by: Petr Wozniak <petr.wozniak@gmail.com>
---
drivers/net/phy/sfp.c | 79 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 73 insertions(+), 6 deletions(-)
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index XXXXXXX..XXXXXXX 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -960,11 +960,88 @@ static void sfp_fixup_rollball(struct sfp *sfp)
sfp->phy_t_retry = msecs_to_jiffies(1000);
}
-static void sfp_fixup_rollball_cc(struct sfp *sfp)
+/* Local mirrors of RollBall protocol constants from mdio-i2c.c */
+#define SFP_ROLLBALL_PHY_ADDR 0x51
+#define SFP_ROLLBALL_MDIO_PAGE 3
+#define SFP_ROLLBALL_CMD_ADDR 0x80
+#define SFP_ROLLBALL_CMD_READ 0x02
+#define SFP_ROLLBALL_CMD_DONE 0x04
+
+static int sfp_rollball_a2_write(struct sfp *sfp, u8 reg,
+ const u8 *data, int len)
+{
+ struct i2c_msg msg;
+ u8 buf[8];
+
+ buf[0] = reg;
+ memcpy(buf + 1, data, len);
+ msg.addr = SFP_ROLLBALL_PHY_ADDR;
+ msg.flags = 0;
+ msg.len = len + 1;
+ msg.buf = buf;
+ return i2c_transfer(sfp->i2c, &msg, 1) == 1 ? 0 : -EIO;
+}
+
+static int sfp_rollball_a2_read(struct sfp *sfp, u8 reg, u8 *val)
{
- sfp_fixup_rollball(sfp);
+ struct i2c_msg msgs[2];
+
+ msgs[0].addr = SFP_ROLLBALL_PHY_ADDR;
+ msgs[0].flags = 0;
+ msgs[0].len = 1;
+ msgs[0].buf = ®
+ msgs[1].addr = SFP_ROLLBALL_PHY_ADDR;
+ msgs[1].flags = I2C_M_RD;
+ msgs[1].len = 1;
+ msgs[1].buf = val;
+ return i2c_transfer(sfp->i2c, msgs, 2) == 2 ? 0 : -EIO;
+}
+
+/**
+ * sfp_has_rollball_bridge() - probe for a RollBall I2C-to-MDIO bridge
+ * @sfp: SFP instance
+ *
+ * Send the RollBall unlock password, switch to the MDIO page, issue CMD_READ
+ * and poll for CMD_DONE. A genuine RollBall bridge asserts CMD_DONE within
+ * ~70 ms. Modules without a bridge (e.g. RTL8261BE pure media converter)
+ * never respond; the poll times out after 200 ms.
+ *
+ * Returns true if a RollBall bridge is present, false otherwise.
+ */
+static bool sfp_has_rollball_bridge(struct sfp *sfp)
+{
+ u8 pw[4] = { 0xff, 0xff, 0xff, 0xff };
+ u8 page = SFP_ROLLBALL_MDIO_PAGE;
+ u8 cmd = SFP_ROLLBALL_CMD_READ;
+ u8 saved_page = 0, res;
+ int i;
+
+ if (sfp_rollball_a2_write(sfp, SFP_VSL + 3, pw, sizeof(pw)) < 0)
+ return false;
+
+ sfp_rollball_a2_read(sfp, SFP_PAGE, &saved_page);
+
+ if (sfp_rollball_a2_write(sfp, SFP_PAGE, &page, 1) < 0 ||
+ sfp_rollball_a2_write(sfp, SFP_ROLLBALL_CMD_ADDR, &cmd, 1) < 0)
+ goto restore;
+
+ for (i = 0; i < 10; i++) {
+ msleep(20);
+ if (!sfp_rollball_a2_read(sfp, SFP_ROLLBALL_CMD_ADDR, &res) &&
+ res == SFP_ROLLBALL_CMD_DONE) {
+ sfp_rollball_a2_write(sfp, SFP_PAGE, &saved_page, 1);
+ return true;
+ }
+ }
+
+restore:
+ sfp_rollball_a2_write(sfp, SFP_PAGE, &saved_page, 1);
+ return false;
+}
+
+static void sfp_fixup_rollball_cc(struct sfp *sfp)
{
+ /* Probe for an I2C-to-MDIO bridge: genuine RollBall modules assert
+ * CMD_DONE within ~70 ms; pure media converters such as the RTL8261BE
+ * have no bridge and time out after 200 ms.
+ */
+ if (!sfp_has_rollball_bridge(sfp)) {
+ sfp->mdio_protocol = MDIO_I2C_NONE;
+ return;
+ }
+ sfp_fixup_rollball(sfp);
/* Some RollBall SFPs may have wrong (zero) extended compliance code
* burned in EEPROM. For PHY probing we need the correct one.
*/
@@ -1036,7 +1113,8 @@ static const struct sfp_quirk sfp_quirks[] = {
SFP_QUIRK_S("OEM", "SFP-GE-T", sfp_fixup_ignore_tx_fault),
SFP_QUIRK_S("OEM", "SFP-2.5G-T", sfp_quirk_oem_2_5g),
SFP_QUIRK_S("OEM", "SFP-2.5G-BX10-D", sfp_quirk_2500basex),
- SFP_QUIRK_F("OEM", "SFP-10G-T", sfp_fixup_rollball_cc),
+ SFP_QUIRK_F("OEM", "SFP-10G-T-I", sfp_fixup_rollball_cc),
+ SFP_QUIRK_F("OEM", "SFP-10G-T", sfp_fixup_rollball_cc),
SFP_QUIRK_S("OEM", "SFP-2.5G-BX10-U", sfp_quirk_2500basex),
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 4/5] phy: qcom: qmp: Enable ipq5210 support
From: Konrad Dybcio @ 2026-05-15 11:29 UTC (permalink / raw)
To: Varadarajan Narayanan, Vinod Koul, Neil Armstrong, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Wesley Cheng,
Greg Kroah-Hartman
Cc: linux-arm-msm, linux-phy, devicetree, linux-kernel, linux-usb
In-Reply-To: <20260515-usb2phy-v1-4-5f8338d466bf@oss.qualcomm.com>
On 5/15/26 12:47 PM, Varadarajan Narayanan wrote:
> Enable QMP USB3 phy support for ipq5210 SoC.
>
> Signed-off-by: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>
> ---
Since they seem fully compatible (also in patch 5), could we simply
use a fallback compatible?
Konrad
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 5/5] phy: qcom-qusb2: add QUSB2 support for ipq5210
From: sashiko-bot @ 2026-05-15 11:28 UTC (permalink / raw)
To: Varadarajan Narayanan
Cc: linux-phy, vkoul, krzk+dt, conor+dt, neil.armstrong, olteanv,
robh, devicetree
In-Reply-To: <20260515-usb2phy-v1-5-5f8338d466bf@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message incorrectly describes the patch contents. It claims to add an init sequence for 'Super Speed ports', but the patch actually modifies the QUSB2 PHY driver, which is responsible for USB 2.0 (High Speed / Full Speed / Low Speed) ports. Furthermore, the patch does not introduce a new initialization sequence; it simply maps the new `qcom,ipq5210-qusb2-phy` compatible string to the existing `ipq6018_phy_cfg` data.
--
commit a327055af09a1520cf8f88735b110f1df7674e64
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>
phy: qcom-qusb2: add QUSB2 support for ipq5210
This commit adds a new compatible string for the IPQ5210 SoC to the QUSB2
PHY driver. It maps the new compatible string to the existing
ipq6018_phy_cfg data structure.
> Add the phy init sequence for the Super Speed ports found
> on ipq5210.
Does this commit message accurately describe the patch contents?
The QUSB2 PHY driver is responsible for USB 2.0 ports, rather than Super
Speed ports. Additionally, the patch maps the compatible string to the
existing ipq6018_phy_cfg data instead of adding a new initialization
sequence.
Could the commit message be updated to reflect that it adds USB 2.0 PHY
support?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-usb2phy-v1-0-5f8338d466bf@oss.qualcomm.com?part=5
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v2 phy-next 2/2] phy: ti: add PHY driver for TI DS125DF111 Dual-Channel Retimer
From: sashiko-bot @ 2026-05-15 11:22 UTC (permalink / raw)
To: Ioana Ciornei
Cc: conor+dt, krzk+dt, devicetree, linux-phy, olteanv, neil.armstrong,
vkoul, robh
In-Reply-To: <20260515110145.1925579-3-ioana.ciornei@nxp.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing delay or polling after setting a self-clearing hardware reset bit.
--
commit 59a5ca7411308a81c146d660b8c04e0be8fe8fbb
Author: Ioana Ciornei <ioana.ciornei@nxp.com>
phy: ti: add PHY driver for TI DS125DF111 Dual-Channel Retimer
This commit adds a generic PHY driver for the TI DS125DF111 Multi-Protocol
Dual-Channel Retimer. It supports 10G and 1G link speeds, modeling each of
the two channels as a separate PHY device.
> diff --git a/drivers/phy/ti/phy-ds125df111.c b/drivers/phy/ti/phy-ds125df111.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/phy/ti/phy-ds125df111.c
[ ... ]
> +static int ds125df111_configure(struct phy *phy,
> + const struct ds125df111_config *cfg)
> +{
[ ... ]
> + /* Reset Channel Registers */
> + err = i2c_smbus_read_byte_data(i2c, DS125DF111_CH_CTRL);
> + if (err < 0) {
> + dev_err(dev, "Error resetting channel configuration\n");
> + goto out;
> + }
> + val = (u8)err;
> + val |= DS125DF111_CH_CTRL_RESET;
> + err = i2c_smbus_write_byte_data(i2c, DS125DF111_CH_CTRL, val);
> + if (err < 0) {
> + dev_err(dev, "Error resetting channel configuration\n");
> + goto out;
> + }
> +
> + /* Program the VCO group frequencies */
> + for (i = 0; i < DS125DF111_NUM_VCO_GROUP_REG; i++) {
> + err = i2c_smbus_write_byte_data(i2c,
> + DS125DF111_VCO_GROUP_BASE + i,
> + cfg->vco_group[i]);
Is a delay or polling loop needed after triggering the channel reset?
The DS125DF111_CH_CTRL_RESET bit is defined as self-clearing, but the code
immediately proceeds to program the VCO group frequencies without checking if
the reset has finished.
Since hardware resets usually take a non-zero amount of time, writing to the
registers right away could result in the device NACKing the transactions or
silently dropping the new configuration.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515110145.1925579-1-ioana.ciornei@nxp.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 3/5] dt-bindings: usb: qcom,dwc3: Add ipq5210 to USB DWC3 bindings
From: sashiko-bot @ 2026-05-15 11:18 UTC (permalink / raw)
To: Varadarajan Narayanan
Cc: krzk+dt, linux-phy, robh, devicetree, vkoul, neil.armstrong,
conor+dt, olteanv
In-Reply-To: <20260515-usb2phy-v1-3-5f8338d466bf@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The new compatible string `qcom,ipq5210-dwc3` is added to the top-level `compatible` list but is omitted from the `allOf` conditional blocks that enforce device-specific constraints.
--
commit f7f00d62e8a27abcab7835184425ce9a31f1b2c3
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>
dt-bindings: usb: qcom,dwc3: Add ipq5210 to USB DWC3 bindings
Update dt-bindings to add ipq5210 to USB DWC3 controller list.
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> index a7f58114c02e8..f8e0819be894b 100644
> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> @@ -26,6 +26,7 @@ properties:
> - enum:
> - qcom,ipq4019-dwc3
> - qcom,ipq5018-dwc3
> + - qcom,ipq5210-dwc3
Should this new compatible string also be added to the allOf conditional blocks
that enforce constraints for related hardware?
Similar generations like ipq5018 and ipq5332 enforce clock and interrupt
limits further down in the file:
allOf:
- if:
properties:
compatible:
contains:
enum:
- qcom,ipq5018-dwc3
- qcom,ipq5332-dwc3
then:
properties:
interrupts:
maxItems: 3
Does ipq5210 share these constraints, and should it be added to the appropriate
conditionals?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-usb2phy-v1-0-5f8338d466bf@oss.qualcomm.com?part=3
--
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