* Re: [PATCH v4 2/2] arm64: dts: qcom: lemans: Add eDP ref clock for eDP PHYs
From: Konrad Dybcio @ 2026-01-28 12:41 UTC (permalink / raw)
To: Ritesh Kumar, robin.clark, lumag, abhinav.kumar, sean,
marijn.suijten, maarten.lankhorst, mripard, tzimmermann, airlied,
simona, robh, krzk+dt, conor+dt, quic_mahap, andersson,
konradybcio, mani, James.Bottomley, martin.petersen, vkoul,
kishon, cros-qcom-dts-watchers
Cc: linux-phy, linux-arm-msm, dri-devel, freedreno, devicetree,
linux-kernel, linux-scsi, quic_vproddut
In-Reply-To: <20260128114853.2543416-3-quic_riteshk@quicinc.com>
On 1/28/26 12:48 PM, Ritesh Kumar wrote:
> The eDP PHY nodes on lemans were missing the reference clock voting.
> This initially went unnoticed because the clock was implicitly enabled
> by the UFS PHY driver, and the eDP PHY happened to rely on that.
>
> After commit 77d2fa54a945 ("scsi: ufs: qcom : Refactor phy_power_on/off
> calls"), the UFS driver no longer keeps the reference clock enabled.
> As a result, the eDP PHY fails to power on.
>
> To fix this, add eDP reference clock for eDP PHYs on lemans chipset
> ensuring reference clock is enabled.
>
> Fixes: e1e3e5673f8d7 ("arm64: dts: qcom: sa8775p: add DisplayPort device nodes")
> Signed-off-by: Ritesh Kumar <quic_riteshk@quicinc.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH v7 1/7] phy: can-transceiver: rename temporary helper function to avoid conflict
From: Josua Mayer @ 2026-01-28 14:46 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Peter Rosin, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Janusz Krzysztofik, Vignesh R,
Andi Shyti, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Geert Uytterhoeven, Magnus Damm, Wolfram Sang
Cc: Yazan Shhady, Jon Nettleton, Mikhail Anikin, linux-can, linux-phy,
linux-kernel, linux-omap, linux-i2c, linux-mmc, devicetree,
linux-renesas-soc, Josua Mayer
In-Reply-To: <20260128-rz-sdio-mux-v7-0-92ebb6da0df8@solid-run.com>
Rename the temporary devm_mux_state_get_optional function to avoid
conflict with upcoming implementation in multiplexer subsystem.
Acked-by: Vinod Koul <vkoul@kernel.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
drivers/phy/phy-can-transceiver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
index 330356706ad7..81591d247128 100644
--- a/drivers/phy/phy-can-transceiver.c
+++ b/drivers/phy/phy-can-transceiver.c
@@ -128,7 +128,7 @@ MODULE_DEVICE_TABLE(of, can_transceiver_phy_ids);
/* Temporary wrapper until the multiplexer subsystem supports optional muxes */
static inline struct mux_state *
-devm_mux_state_get_optional(struct device *dev, const char *mux_name)
+temp_devm_mux_state_get_optional(struct device *dev, const char *mux_name)
{
if (!of_property_present(dev->of_node, "mux-states"))
return NULL;
@@ -183,7 +183,7 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
priv->num_ch = num_ch;
platform_set_drvdata(pdev, priv);
- mux_state = devm_mux_state_get_optional(dev, NULL);
+ mux_state = temp_devm_mux_state_get_optional(dev, NULL);
if (IS_ERR(mux_state))
return PTR_ERR(mux_state);
--
2.43.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v7 0/7] mmc: host: renesas_sdhi_core: support configuring an optional sdio mux
From: Josua Mayer @ 2026-01-28 14:46 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Peter Rosin, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Janusz Krzysztofik, Vignesh R,
Andi Shyti, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Geert Uytterhoeven, Magnus Damm, Wolfram Sang
Cc: Yazan Shhady, Jon Nettleton, Mikhail Anikin, linux-can, linux-phy,
linux-kernel, linux-omap, linux-i2c, linux-mmc, devicetree,
linux-renesas-soc, Josua Mayer
Some Renesas SoC based boards mux SD and eMMC on a single sdio
controller, exposing user control by dip switch and software control by
gpio.
Purpose is to simplify development and provisioning by selecting boot
media at power-on, and again before starting linux.
Add binding and driver support for linking a (gpio) mux to renesas sdio
controller.
Introduce generic helper functions for getting managed and selected
mux-state objects, and switch i2c-omap and phy-can-transceiver drivers.
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
Changes in v7:
- picked up reviewed-tags
- fix Kconfig change to add the missing prompt for CONFIG_MULTIPLEXER,
and enable it by default when COMPILE_TEST is set.
(Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>)
- fix another kernel build robot warning: undocumented C struct member
- Link to v6: https://lore.kernel.org/r/20260121-rz-sdio-mux-v6-0-38aa39527928@solid-run.com
Changes in v6:
- replaced /* with /** for devm_mux_state_state function description.
- collected review tags.
- fixed checkpatch warnings (space-before-tab, void-return).
(Reported-by: Geert Uytterhoeven)
- fixed use-after-free in mux core mux_get function.
(Reported-by: Geert Uytterhoeven)
- fix mux helper error path uninitialised return code variable.
(Reported-by: kernel test robot <lkp@intel.com>)
- Link to v5: https://lore.kernel.org/r/20260118-rz-sdio-mux-v5-0-3c37e8872683@solid-run.com
Changes in v5:
- implemented automatic mux deselect for devm_*_selected.
(Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>)
- because of semantic changes I dropped reviewed and acks from omap-i2c
patch (Andreas Kemnade / Wolfram Sang).
- fix invalid return value in void function for mux helper stubs
(Reported-by: kernel test robot <lkp@intel.com>)
- Link to v4: https://lore.kernel.org/r/20251229-rz-sdio-mux-v4-0-a023e55758fe@solid-run.com
Changes in v4:
- added MULTIPLEXER Kconfig help text.
- removed "select MULTIPLEXER" from renesas sdhi Kconfig, as it is
not required for all devices using this driver.
- added stubs for all symbols exported by mux core.
(Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>)
- refactored mux core logic to silence ENOENT errors only on optional
code paths, keeping error printing unchanged otherwise.
(Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>)
- picked up various reviewed- and acked-by tags
- Link to v3: https://lore.kernel.org/r/20251210-rz-sdio-mux-v3-0-ca628db56d60@solid-run.com
Changes in v3:
- updated omap-i2c and phy-can-transceiver to use new helpers.
- created generic helper functions for getting managed optional mux-state.
(Reported-by: Rob Herring <robh@kernel.org>)
- picked up binding ack by Rob Herring.
- replaced use of "SDIO" with "SD/SDIO/eMMC" in binding document and
commit descriptions.
(Reported-by: Ulf Hansson <ulf.hansson@linaro.org>)
- Link to v2: https://lore.kernel.org/r/20251201-rz-sdio-mux-v2-0-bcb581b88dd7@solid-run.com
Changes in v2:
- dropped mux-controller node from dt binding example
(Reported-by: Conor Dooley <conor@kernel.org>
Reported-by: Krzysztof Kozlowski <krzk@kernel.org>)
- Link to v1: https://lore.kernel.org/r/20251128-rz-sdio-mux-v1-0-1ede318d160f@solid-run.com
---
Josua Mayer (7):
phy: can-transceiver: rename temporary helper function to avoid conflict
mux: Add helper functions for getting optional and selected mux-state
mux: add help text for MULTIPLEXER config option
phy: can-transceiver: drop temporary helper getting optional mux-state
i2c: omap: switch to new generic helper for getting selected mux-state
dt-bindings: mmc: renesas,sdhi: Add mux-states property
mmc: host: renesas_sdhi_core: support selecting an optional mux
.../devicetree/bindings/mmc/renesas,sdhi.yaml | 6 +
drivers/i2c/busses/i2c-omap.c | 24 +--
drivers/mmc/host/renesas_sdhi_core.c | 6 +
drivers/mux/Kconfig | 8 +-
drivers/mux/core.c | 178 +++++++++++++++++----
drivers/phy/phy-can-transceiver.c | 10 --
include/linux/mux/consumer.h | 108 ++++++++++++-
7 files changed, 277 insertions(+), 63 deletions(-)
---
base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
change-id: 20251128-rz-sdio-mux-acc5137f1618
Best regards,
--
Josua Mayer <josua@solid-run.com>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH v7 3/7] mux: add help text for MULTIPLEXER config option
From: Josua Mayer @ 2026-01-28 14:46 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Peter Rosin, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Janusz Krzysztofik, Vignesh R,
Andi Shyti, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Geert Uytterhoeven, Magnus Damm, Wolfram Sang
Cc: Yazan Shhady, Jon Nettleton, Mikhail Anikin, linux-can, linux-phy,
linux-kernel, linux-omap, linux-i2c, linux-mmc, devicetree,
linux-renesas-soc, Josua Mayer
In-Reply-To: <20260128-rz-sdio-mux-v7-0-92ebb6da0df8@solid-run.com>
Add prompt and help text for CONFIG_MULTIPLEXER to allow enabling this
option thorugh the kernel configuration without explicit "select" driver
dependencies.
Select it by default when COMPILE_TEST is set for better coverage.
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
drivers/mux/Kconfig | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
index c68132e38138..e31c46820bdf 100644
--- a/drivers/mux/Kconfig
+++ b/drivers/mux/Kconfig
@@ -4,7 +4,13 @@
#
config MULTIPLEXER
- tristate
+ tristate "Generic Multiplexer Support" if COMPILE_TEST
+ help
+ This framework is designed to abstract multiplexer handling for
+ devices via various GPIO-, MMIO/Regmap or specific multiplexer
+ controller chips.
+
+ If unsure, say no.
menu "Multiplexer drivers"
depends on MULTIPLEXER
--
2.43.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v7 2/7] mux: Add helper functions for getting optional and selected mux-state
From: Josua Mayer @ 2026-01-28 14:46 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Peter Rosin, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Janusz Krzysztofik, Vignesh R,
Andi Shyti, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Geert Uytterhoeven, Magnus Damm, Wolfram Sang
Cc: Yazan Shhady, Jon Nettleton, Mikhail Anikin, linux-can, linux-phy,
linux-kernel, linux-omap, linux-i2c, linux-mmc, devicetree,
linux-renesas-soc, Josua Mayer
In-Reply-To: <20260128-rz-sdio-mux-v7-0-92ebb6da0df8@solid-run.com>
In-tree phy-can-transceiver driver has already implemented a local
version of devm_mux_state_get_optional.
The omap-i2c driver gets and selects an optional mux in its probe
function without using any helper.
Add new helper functions covering both aforementioned use-cases:
- mux_control_get_optional:
Get a mux-control if specified in dt, return NULL otherwise.
- devm_mux_state_get_optional:
Get a mux-state if specified in dt, return NULL otherwise.
- devm_mux_state_get_selected:
Get and select a mux-state specified in dt, return error otherwise.
- devm_mux_state_get_optional_selected:
Get and select a mux-state if specified in dt, return error or NULL.
Existing mux_get helper function is changed to take an extra argument
indicating whether the mux is optional.
In this case no error is printed, and NULL returned in case of ENOENT.
Calling code is adapted to handle NULL return case, and to pass optional
argument as required.
To support automatic deselect for _selected helper, a new structure is
created storing an exit pointer similar to clock core which is called on
release.
To facilitate code sharing between optional/mandatory/selected helpers,
a new internal helper function is added to handle quiet (optional) and
verbose (mandatory) errors, as well as storing the correct callback for
devm release: __devm_mux_state_get
Due to this structure devm_mux_state_get_*_selected can no longer print
a useful error message when select fails. Instead callers should print
errors where needed.
Commit e153fdea9db04 ("phy: can-transceiver: Re-instate "mux-states"
property presence check") noted that "mux_get() always prints an error
message in case of an error, including when the property is not present,
confusing the user."
The first error message covers the case that a mux name is not matched
in dt. The second error message is based on of_parse_phandle_with_args
return value.
In optional case no error is printed and NULL is returned.
This ensures that the new helper functions will not confuse the user
either.
With the addition of optional helper functions it became clear that
drivers should compile and link even if CONFIG_MULTIPLEXER was not enabled.
Add stubs for all symbols exported by mux core.
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
drivers/mux/core.c | 178 ++++++++++++++++++++++++++++++++++++-------
include/linux/mux/consumer.h | 108 +++++++++++++++++++++++++-
2 files changed, 253 insertions(+), 33 deletions(-)
diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index a3840fe0995f..b01ec126caaf 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -46,6 +46,16 @@ static const struct class mux_class = {
.name = "mux",
};
+/**
+ * struct devm_mux_state_state - Tracks managed resources for mux-state objects.
+ * @mstate: Pointer to a mux state.
+ * @exit: An optional callback to execte before free.
+ */
+struct devm_mux_state_state {
+ struct mux_state *mstate;
+ int (*exit)(struct mux_state *mstate);
+};
+
static DEFINE_IDA(mux_ida);
static int __init mux_init(void)
@@ -522,11 +532,12 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
* @mux_name: The name identifying the mux-control.
* @state: Pointer to where the requested state is returned, or NULL when
* the required multiplexer states are handled by other means.
+ * @optional: Whether to return NULL and silence errors when mux doesn't exist.
*
* Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
*/
static struct mux_control *mux_get(struct device *dev, const char *mux_name,
- unsigned int *state)
+ unsigned int *state, bool optional)
{
struct device_node *np = dev->of_node;
struct of_phandle_args args;
@@ -542,7 +553,9 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
else
index = of_property_match_string(np, "mux-control-names",
mux_name);
- if (index < 0) {
+ if (index < 0 && optional) {
+ return NULL;
+ } else if (index < 0) {
dev_err(dev, "mux controller '%s' not found\n",
mux_name);
return ERR_PTR(index);
@@ -558,8 +571,12 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
"mux-controls", "#mux-control-cells",
index, &args);
if (ret) {
+ if (optional && ret == -ENOENT)
+ return NULL;
+
dev_err(dev, "%pOF: failed to get mux-%s %s(%i)\n",
- np, state ? "state" : "control", mux_name ?: "", index);
+ np, state ? "state" : "control",
+ mux_name ?: "", index);
return ERR_PTR(ret);
}
@@ -617,10 +634,23 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
*/
struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
{
- return mux_get(dev, mux_name, NULL);
+ return mux_get(dev, mux_name, NULL, false);
}
EXPORT_SYMBOL_GPL(mux_control_get);
+/**
+ * mux_control_get_optional() - Get the optional mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: A pointer to the mux-control, an ERR_PTR with a negative errno.
+ */
+struct mux_control *mux_control_get_optional(struct device *dev, const char *mux_name)
+{
+ return mux_get(dev, mux_name, NULL, true);
+}
+EXPORT_SYMBOL_GPL(mux_control_get_optional);
+
/**
* mux_control_put() - Put away the mux-control for good.
* @mux: The mux-control to put away.
@@ -657,8 +687,8 @@ struct mux_control *devm_mux_control_get(struct device *dev,
if (!ptr)
return ERR_PTR(-ENOMEM);
- mux = mux_control_get(dev, mux_name);
- if (IS_ERR(mux)) {
+ mux = mux_get(dev, mux_name, NULL, false);
+ if (IS_ERR_OR_NULL(mux)) {
devres_free(ptr);
return mux;
}
@@ -677,20 +707,19 @@ EXPORT_SYMBOL_GPL(devm_mux_control_get);
*
* Return: A pointer to the mux-state, or an ERR_PTR with a negative errno.
*/
-static struct mux_state *mux_state_get(struct device *dev, const char *mux_name)
+static struct mux_state *mux_state_get(struct device *dev, const char *mux_name, bool optional)
{
struct mux_state *mstate;
+ struct mux_control *mux;
mstate = kzalloc(sizeof(*mstate), GFP_KERNEL);
if (!mstate)
return ERR_PTR(-ENOMEM);
- mstate->mux = mux_get(dev, mux_name, &mstate->state);
- if (IS_ERR(mstate->mux)) {
- int err = PTR_ERR(mstate->mux);
-
+ mstate->mux = mux = mux_get(dev, mux_name, &mstate->state, optional);
+ if (IS_ERR_OR_NULL(mux)) {
kfree(mstate);
- return ERR_PTR(err);
+ return ERR_CAST(mux);
}
return mstate;
@@ -710,41 +739,132 @@ static void mux_state_put(struct mux_state *mstate)
static void devm_mux_state_release(struct device *dev, void *res)
{
- struct mux_state *mstate = *(struct mux_state **)res;
+ struct devm_mux_state_state *devm_state = res;
- mux_state_put(mstate);
+ if (devm_state->exit)
+ devm_state->exit(devm_state->mstate);
+
+ mux_state_put(devm_state->mstate);
}
/**
- * devm_mux_state_get() - Get the mux-state for a device, with resource
- * management.
- * @dev: The device that needs a mux-control.
- * @mux_name: The name identifying the mux-control.
+ * __devm_mux_state_get() - Get the optional mux-state for a device,
+ * with resource management.
+ * @dev: The device that needs a mux-state.
+ * @mux_name: The name identifying the mux-state.
+ * @optional: Whether to return NULL and silence errors when mux doesn't exist.
+ * @init: Optional function pointer for mux-state object initialisation.
+ * @exit: Optional function pointer for mux-state object cleanup on release.
*
* Return: Pointer to the mux-state, or an ERR_PTR with a negative errno.
*/
-struct mux_state *devm_mux_state_get(struct device *dev,
- const char *mux_name)
+static struct mux_state *__devm_mux_state_get(struct device *dev, const char *mux_name,
+ bool optional,
+ int (*init)(struct mux_state *mstate),
+ int (*exit)(struct mux_state *mstate))
{
- struct mux_state **ptr, *mstate;
+ struct devm_mux_state_state *devm_state;
+ struct mux_state *mstate;
+ int ret;
- ptr = devres_alloc(devm_mux_state_release, sizeof(*ptr), GFP_KERNEL);
- if (!ptr)
+ devm_state = devres_alloc(devm_mux_state_release, sizeof(*devm_state), GFP_KERNEL);
+ if (!devm_state)
return ERR_PTR(-ENOMEM);
- mstate = mux_state_get(dev, mux_name);
- if (IS_ERR(mstate)) {
- devres_free(ptr);
- return mstate;
+ mstate = mux_state_get(dev, mux_name, optional);
+ if (IS_ERR_OR_NULL(mstate)) {
+ ret = PTR_ERR(mstate);
+ goto err_mux_state_get;
}
- *ptr = mstate;
- devres_add(dev, ptr);
+ if (init) {
+ ret = init(mstate);
+ if (ret)
+ goto err_mux_state_init;
+ }
+
+ devm_state->mstate = mstate;
+ devm_state->exit = exit;
+ devres_add(dev, devm_state);
return mstate;
+
+err_mux_state_init:
+ mux_state_put(mstate);
+err_mux_state_get:
+ devres_free(devm_state);
+ return ERR_PTR(ret);
+}
+
+/**
+ * devm_mux_state_get() - Get the mux-state for a device, with resource
+ * management.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: Pointer to the mux-state, or an ERR_PTR with a negative errno.
+ *
+ * The mux-state will automatically be freed on release.
+ */
+struct mux_state *devm_mux_state_get(struct device *dev, const char *mux_name)
+{
+ return __devm_mux_state_get(dev, mux_name, false, NULL, NULL);
}
EXPORT_SYMBOL_GPL(devm_mux_state_get);
+/**
+ * devm_mux_state_get_optional() - Get the optional mux-state for a device,
+ * with resource management.
+ * @dev: The device that needs a mux-state.
+ * @mux_name: The name identifying the mux-state.
+ *
+ * Return: Pointer to the mux-state, or an ERR_PTR with a negative errno.
+ *
+ * The mux-state will automatically be freed on release.
+ */
+struct mux_state *devm_mux_state_get_optional(struct device *dev, const char *mux_name)
+{
+ return __devm_mux_state_get(dev, mux_name, true, NULL, NULL);
+}
+EXPORT_SYMBOL_GPL(devm_mux_state_get_optional);
+
+/**
+ * devm_mux_state_get_selected() - Get the mux-state for a device, with
+ * resource management.
+ * @dev: The device that needs a mux-state.
+ * @mux_name: The name identifying the mux-state.
+ *
+ * Return: Pointer to the mux-state, or an ERR_PTR with a negative errno.
+ *
+ * The returned mux-state (if valid) is already selected.
+ *
+ * The mux-state will automatically be deselected and freed on release.
+ */
+struct mux_state *devm_mux_state_get_selected(struct device *dev, const char *mux_name)
+{
+ return __devm_mux_state_get(dev, mux_name, false, mux_state_select, mux_state_deselect);
+}
+EXPORT_SYMBOL_GPL(devm_mux_state_get_selected);
+
+/**
+ * devm_mux_state_get_optional_selected() - Get the optional mux-state for
+ * a device, with resource management.
+ * @dev: The device that needs a mux-state.
+ * @mux_name: The name identifying the mux-state.
+ *
+ * Return: Pointer to the mux-state, or an ERR_PTR with a negative errno.
+ *
+ * The returned mux-state (if valid) is already selected.
+ *
+ * The mux-state will automatically be deselected and freed on release.
+ */
+struct mux_state *devm_mux_state_get_optional_selected(struct device *dev,
+ const char *mux_name)
+{
+ return __devm_mux_state_get(dev, mux_name, true, mux_state_select, mux_state_deselect);
+}
+EXPORT_SYMBOL_GPL(devm_mux_state_get_optional_selected);
+
/*
* Using subsys_initcall instead of module_init here to try to ensure - for
* the non-modular case - that the subsystem is initialized when mux consumers
diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
index 2e25c838f831..3ede55b907eb 100644
--- a/include/linux/mux/consumer.h
+++ b/include/linux/mux/consumer.h
@@ -16,6 +16,8 @@ struct device;
struct mux_control;
struct mux_state;
+#ifdef CONFIG_MULTIPLEXER
+
unsigned int mux_control_states(struct mux_control *mux);
int __must_check mux_control_select_delay(struct mux_control *mux,
unsigned int state,
@@ -54,11 +56,109 @@ int mux_control_deselect(struct mux_control *mux);
int mux_state_deselect(struct mux_state *mstate);
struct mux_control *mux_control_get(struct device *dev, const char *mux_name);
+struct mux_control *mux_control_get_optional(struct device *dev, const char *mux_name);
void mux_control_put(struct mux_control *mux);
-struct mux_control *devm_mux_control_get(struct device *dev,
- const char *mux_name);
-struct mux_state *devm_mux_state_get(struct device *dev,
- const char *mux_name);
+struct mux_control *devm_mux_control_get(struct device *dev, const char *mux_name);
+struct mux_state *devm_mux_state_get(struct device *dev, const char *mux_name);
+struct mux_state *devm_mux_state_get_optional(struct device *dev, const char *mux_name);
+struct mux_state *devm_mux_state_get_selected(struct device *dev, const char *mux_name);
+struct mux_state *devm_mux_state_get_optional_selected(struct device *dev, const char *mux_name);
+
+#else
+
+static inline unsigned int mux_control_states(struct mux_control *mux)
+{
+ return 0;
+}
+static inline int __must_check mux_control_select_delay(struct mux_control *mux,
+ unsigned int state, unsigned int delay_us)
+{
+ return -EOPNOTSUPP;
+}
+static inline int __must_check mux_state_select_delay(struct mux_state *mstate,
+ unsigned int delay_us)
+{
+ return -EOPNOTSUPP;
+}
+static inline int __must_check mux_control_try_select_delay(struct mux_control *mux,
+ unsigned int state,
+ unsigned int delay_us)
+{
+ return -EOPNOTSUPP;
+}
+static inline int __must_check mux_state_try_select_delay(struct mux_state *mstate,
+ unsigned int delay_us)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int __must_check mux_control_select(struct mux_control *mux,
+ unsigned int state)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int __must_check mux_state_select(struct mux_state *mstate)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int __must_check mux_control_try_select(struct mux_control *mux,
+ unsigned int state)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int __must_check mux_state_try_select(struct mux_state *mstate)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int mux_control_deselect(struct mux_control *mux)
+{
+ return -EOPNOTSUPP;
+}
+static inline int mux_state_deselect(struct mux_state *mstate)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+static inline struct mux_control *mux_control_get_optional(struct device *dev,
+ const char *mux_name)
+{
+ return NULL;
+}
+static inline void mux_control_put(struct mux_control *mux) {}
+
+static inline struct mux_control *devm_mux_control_get(struct device *dev, const char *mux_name)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+static inline struct mux_state *devm_mux_state_get(struct device *dev, const char *mux_name)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+static inline struct mux_state *devm_mux_state_get_optional(struct device *dev,
+ const char *mux_name)
+{
+ return NULL;
+}
+static inline struct mux_state *devm_mux_state_get_selected(struct device *dev,
+ const char *mux_name)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
+static inline struct mux_state *devm_mux_state_get_optional_selected(struct device *dev,
+ const char *mux_name)
+{
+ return NULL;
+}
+
+#endif /* CONFIG_MULTIPLEXER */
#endif /* _LINUX_MUX_CONSUMER_H */
--
2.43.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v7 4/7] phy: can-transceiver: drop temporary helper getting optional mux-state
From: Josua Mayer @ 2026-01-28 14:46 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Peter Rosin, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Janusz Krzysztofik, Vignesh R,
Andi Shyti, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Geert Uytterhoeven, Magnus Damm, Wolfram Sang
Cc: Yazan Shhady, Jon Nettleton, Mikhail Anikin, linux-can, linux-phy,
linux-kernel, linux-omap, linux-i2c, linux-mmc, devicetree,
linux-renesas-soc, Josua Mayer
In-Reply-To: <20260128-rz-sdio-mux-v7-0-92ebb6da0df8@solid-run.com>
Multiplexer subsystem has now added helpers for getting managed optional
mux-state.
Switch to the new devm_mux_state_get_optional helper.
This change is only compile-tested.
Acked-by: Vinod Koul <vkoul@kernel.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
drivers/phy/phy-can-transceiver.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
index 81591d247128..2b52e47f247a 100644
--- a/drivers/phy/phy-can-transceiver.c
+++ b/drivers/phy/phy-can-transceiver.c
@@ -126,16 +126,6 @@ static const struct of_device_id can_transceiver_phy_ids[] = {
};
MODULE_DEVICE_TABLE(of, can_transceiver_phy_ids);
-/* Temporary wrapper until the multiplexer subsystem supports optional muxes */
-static inline struct mux_state *
-temp_devm_mux_state_get_optional(struct device *dev, const char *mux_name)
-{
- if (!of_property_present(dev->of_node, "mux-states"))
- return NULL;
-
- return devm_mux_state_get(dev, mux_name);
-}
-
static struct phy *can_transceiver_phy_xlate(struct device *dev,
const struct of_phandle_args *args)
{
@@ -183,7 +173,7 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
priv->num_ch = num_ch;
platform_set_drvdata(pdev, priv);
- mux_state = temp_devm_mux_state_get_optional(dev, NULL);
+ mux_state = devm_mux_state_get_optional(dev, NULL);
if (IS_ERR(mux_state))
return PTR_ERR(mux_state);
--
2.43.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v7 5/7] i2c: omap: switch to new generic helper for getting selected mux-state
From: Josua Mayer @ 2026-01-28 14:46 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Peter Rosin, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Janusz Krzysztofik, Vignesh R,
Andi Shyti, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Geert Uytterhoeven, Magnus Damm, Wolfram Sang
Cc: Yazan Shhady, Jon Nettleton, Mikhail Anikin, linux-can, linux-phy,
linux-kernel, linux-omap, linux-i2c, linux-mmc, devicetree,
linux-renesas-soc, Josua Mayer
In-Reply-To: <20260128-rz-sdio-mux-v7-0-92ebb6da0df8@solid-run.com>
Multiplexer subsystem has added generic helper functions for getting an
already selected mux-state object.
Replace existing logic in probe with the equivalent helper function.
There is a functional difference in that the mux is now automatically
deselected on release, replacing the explicit mux_state_deselect call.
This change is only compile-tested.
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Andreas Kemnade <andreas@kemnade.info>
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
drivers/i2c/busses/i2c-omap.c | 24 +++++-------------------
1 file changed, 5 insertions(+), 19 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index d9f590f0c384..f02d294db42a 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1453,27 +1453,16 @@ omap_i2c_probe(struct platform_device *pdev)
(1000 * omap->speed / 8);
}
- if (of_property_present(node, "mux-states")) {
- struct mux_state *mux_state;
-
- mux_state = devm_mux_state_get(&pdev->dev, NULL);
- if (IS_ERR(mux_state)) {
- r = PTR_ERR(mux_state);
- dev_dbg(&pdev->dev, "failed to get I2C mux: %d\n", r);
- goto err_put_pm;
- }
- omap->mux_state = mux_state;
- r = mux_state_select(omap->mux_state);
- if (r) {
- dev_err(&pdev->dev, "failed to select I2C mux: %d\n", r);
- goto err_put_pm;
- }
+ omap->mux_state = devm_mux_state_get_optional_selected(&pdev->dev, NULL);
+ if (IS_ERR(omap->mux_state)) {
+ r = PTR_ERR(omap->mux_state);
+ goto err_put_pm;
}
/* reset ASAP, clearing any IRQs */
r = omap_i2c_init(omap);
if (r)
- goto err_mux_state_deselect;
+ goto err_put_pm;
if (omap->rev < OMAP_I2C_OMAP1_REV_2)
r = devm_request_irq(&pdev->dev, omap->irq, omap_i2c_omap1_isr,
@@ -1515,9 +1504,6 @@ omap_i2c_probe(struct platform_device *pdev)
err_unuse_clocks:
omap_i2c_write_reg(omap, OMAP_I2C_CON_REG, 0);
-err_mux_state_deselect:
- if (omap->mux_state)
- mux_state_deselect(omap->mux_state);
err_put_pm:
pm_runtime_put_sync(omap->dev);
err_disable_pm:
--
2.43.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v7 6/7] dt-bindings: mmc: renesas,sdhi: Add mux-states property
From: Josua Mayer @ 2026-01-28 14:46 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Peter Rosin, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Janusz Krzysztofik, Vignesh R,
Andi Shyti, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Geert Uytterhoeven, Magnus Damm, Wolfram Sang
Cc: Yazan Shhady, Jon Nettleton, Mikhail Anikin, linux-can, linux-phy,
linux-kernel, linux-omap, linux-i2c, linux-mmc, devicetree,
linux-renesas-soc, Josua Mayer
In-Reply-To: <20260128-rz-sdio-mux-v7-0-92ebb6da0df8@solid-run.com>
Add mux controller support for data or control lines that are muxed
between a host and multiple cards.
There are several devices supporting a choice of eMMC or SD on a single
board by both dip switch and gpio, e.g. Renesas RZ/G2L SMARC SoM and
SolidRun RZ/G2L SoM.
In-tree dts for the Renesas boards currently rely on preprocessor macros
and gpio hogs to describe the respective cards.
By adding mux-states property to sdhi controller description, boards can
correctly describe the mux that already exists in hardware - and drivers
can coordinate between mux selection and probing for cards.
Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
Documentation/devicetree/bindings/mmc/renesas,sdhi.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/renesas,sdhi.yaml b/Documentation/devicetree/bindings/mmc/renesas,sdhi.yaml
index c754ea71f51f..64fac0d11329 100644
--- a/Documentation/devicetree/bindings/mmc/renesas,sdhi.yaml
+++ b/Documentation/devicetree/bindings/mmc/renesas,sdhi.yaml
@@ -106,6 +106,11 @@ properties:
iommus:
maxItems: 1
+ mux-states:
+ description:
+ mux controller node to route the SD/SDIO/eMMC signals from SoC to cards.
+ maxItems: 1
+
power-domains:
maxItems: 1
@@ -275,6 +280,7 @@ examples:
max-frequency = <195000000>;
power-domains = <&sysc R8A7790_PD_ALWAYS_ON>;
resets = <&cpg 314>;
+ mux-states = <&mux 0>;
};
sdhi1: mmc@ee120000 {
--
2.43.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v7 7/7] mmc: host: renesas_sdhi_core: support selecting an optional mux
From: Josua Mayer @ 2026-01-28 14:46 UTC (permalink / raw)
To: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Peter Rosin, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Janusz Krzysztofik, Vignesh R,
Andi Shyti, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Geert Uytterhoeven, Magnus Damm, Wolfram Sang
Cc: Yazan Shhady, Jon Nettleton, Mikhail Anikin, linux-can, linux-phy,
linux-kernel, linux-omap, linux-i2c, linux-mmc, devicetree,
linux-renesas-soc, Josua Mayer
In-Reply-To: <20260128-rz-sdio-mux-v7-0-92ebb6da0df8@solid-run.com>
Some hardware designs route data or control signals through a mux to
support multiple devices on a single sdhi controller.
In particular SolidRun RZ/G2L/G2LC/V2L System on Module use a mux for
switching between soldered eMMC and an optional microSD on a carrier
board, e.g. for development or provisioning.
SD/SDIO/eMMC are not well suited for runtime switching between different
cards, however boot-time selection is possible and useful - in
particular considering dt overlays.
Add support for an optional SD/SDIO/eMMC mux defined in dt, and select
it during probe.
Similar functionality already exists in other places, e.g. i2c-omap.
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
drivers/mmc/host/renesas_sdhi_core.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 2a310a145785..f9ec78d699f4 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -26,6 +26,7 @@
#include <linux/mmc/mmc.h>
#include <linux/mmc/slot-gpio.h>
#include <linux/module.h>
+#include <linux/mux/consumer.h>
#include <linux/pinctrl/consumer.h>
#include <linux/pinctrl/pinctrl-state.h>
#include <linux/platform_data/tmio.h>
@@ -1062,6 +1063,7 @@ int renesas_sdhi_probe(struct platform_device *pdev,
struct regulator_dev *rdev;
struct renesas_sdhi_dma *dma_priv;
struct device *dev = &pdev->dev;
+ struct mux_state *mux_state;
struct tmio_mmc_host *host;
struct renesas_sdhi *priv;
int num_irqs, irq, ret, i;
@@ -1116,6 +1118,10 @@ int renesas_sdhi_probe(struct platform_device *pdev,
"state_uhs");
}
+ mux_state = devm_mux_state_get_optional_selected(&pdev->dev, NULL);
+ if (IS_ERR(mux_state))
+ return PTR_ERR(mux_state);
+
host = tmio_mmc_host_alloc(pdev, mmc_data);
if (IS_ERR(host))
return PTR_ERR(host);
--
2.43.0
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* Re: [PATCH v6 3/7] mux: add help text for MULTIPLEXER config option
From: Josua Mayer @ 2026-01-28 14:47 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Peter Rosin, Aaro Koskinen, Andreas Kemnade, Kevin Hilman,
Roger Quadros, Tony Lindgren, Janusz Krzysztofik, Vignesh R,
Andi Shyti, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Geert Uytterhoeven, Magnus Damm, Wolfram Sang,
Yazan Shhady, Jon Nettleton, Mikhail Anikin,
linux-can@vger.kernel.org, linux-phy@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org,
linux-i2c@vger.kernel.org, linux-mmc@vger.kernel.org,
devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org
In-Reply-To: <CAMuHMdX_rKgfusHP5qVny8OZufU6VAiA6sqg1LP3T2jikSz7yQ@mail.gmail.com>
On 21/01/2026 12:59, Geert Uytterhoeven wrote:
> Hi Josua,
>
> On Wed, 21 Jan 2026 at 11:02, Josua Mayer <josua@solid-run.com> wrote:
>> Add help text for CONFIG_MULTIPLEXER to allow enabling this option
>> through the kernel configuration without explicit "select" driver
>> dependencies.
>>
>> Signed-off-by: Josua Mayer <josua@solid-run.com>
> Thanks for your patch!
>
>> --- a/drivers/mux/Kconfig
>> +++ b/drivers/mux/Kconfig
>> @@ -5,6 +5,14 @@
>>
>> config MULTIPLEXER
>> tristate
>> + help
>> + Generic Multiplexer Support.
>> +
>> + This framework is designed to abstract multiplexer handling for
>> + devices via various GPIO-, MMIO/Regmap or specific multiplexer
>> + controller chips.
>> +
>> + If unsure, say no.
>>
>> menu "Multiplexer drivers"
>> depends on MULTIPLEXER
>>
> Unfortunately it doesn't work like that. As the tristate has no prompt
> specified, the user will never be asked about this.
> You should use something like below:
>
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -4,10 +4,8 @@
> #
>
> config MULTIPLEXER
> - tristate
> + tristate "Generic Multiplexer Support" if COMPILE_TEST
> help
> - Generic Multiplexer Support.
> -
> This framework is designed to abstract multiplexer handling for
> devices via various GPIO-, MMIO/Regmap or specific multiplexer
> controller chips.
Thank you, changed with v7
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH next] phy: google: fix build dependency for Google Tensor USB PHY
From: André Draszik @ 2026-01-28 19:57 UTC (permalink / raw)
To: Roy Luo
Cc: Vinod Koul, Neil Armstrong, Peter Griffin, Tudor Ambarus,
Joy Chakraborty, Naveen Kumar, linux-phy, linux-kernel,
linux-arm-kernel, linux-samsung-soc, kernel test robot
In-Reply-To: <CA+zupgztBHTJwNPEOfXjdw9TpcpiVGO=hdFBYk3PUo-0Vf95mg@mail.gmail.com>
Hi Roy,
On Mon, 2026-01-26 at 18:34 -0800, Roy Luo wrote:
> the wrong example. Let's look at this build error scenario with
> "depends on TYPEC || COMPILE_TEST":
> Given COMPILE_TEST=y and CONFIG_TYPEC=m, the
> "depends on" would be evaluated as max(y,m) = y.
> Hence, CONFIG_PHY_GOOGLE_USB is allowed to be
> set to y, m or n. When it's set to y, the exact build error
> would be introduced. I also tested this locally and saw
> the build error.
Thanks for trying Roy. In that case, it should be written as
depends on TYPEC || (TYPEC=n && COMPILE_TEST)
Cheers,
Andre'
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH] phy: PHY_GOOGLE_USB should depend on ARCH_GOOGLE
From: André Draszik @ 2026-01-28 20:03 UTC (permalink / raw)
To: Geert Uytterhoeven, Vinod Koul, Neil Armstrong, Peter Griffin,
Tudor Ambarus, Naveen Kumar, Roy Luo, Joy Chakraborty,
Arnd Bergmann
Cc: linux-phy, linux-arm-kernel, linux-samsung-soc, linux-kernel
In-Reply-To: <0caa3c449b0c3d64944da3f1003d9389bdc13f98.1769541083.git.geert+renesas@glider.be>
Hi Geert,
On Tue, 2026-01-27 at 20:12 +0100, Geert Uytterhoeven wrote:
> The Google Tensor SoC USB PHY is only present on Google Tensor G5
> (Laguna) SoCs. Hence add a dependency on ARCH_GOOGLE, to prevent asking
> the user about this driver when configuring a kernel without Google
> Tensor SoC support.
>
> Fixes: cbce66669c82ee9a ("phy: Add Google Tensor SoC USB PHY driver")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> ARCH_GOOGLE is not yet upstream, but that doesn't hurt.
I can see your point, and have no objection, but for context, Greg
had asked to not add dependencies on a config option that might or
might not get added in the future - ARCH_GOOGLE:
https://lore.kernel.org/all/2025121728-reliably-crabgrass-2601@gregkh/
Maybe an alternative would be to make it default=n, and change that
default once ARCH_GOOGLE gets added?
Cheers,
Andre'
> ---
> drivers/phy/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 142e7b0ef2efb920..3ceda9a20d571038 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -49,6 +49,7 @@ config GENERIC_PHY_MIPI_DPHY
>
> config PHY_GOOGLE_USB
> tristate "Google Tensor SoC USB PHY driver"
> + depends on ARCH_GOOGLE || COMPILE_TEST
> select GENERIC_PHY
> help
> Enable support for the USB PHY on Google Tensor SoCs, starting with
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH next] phy: google: fix build dependency for Google Tensor USB PHY
From: Roy Luo @ 2026-01-28 21:22 UTC (permalink / raw)
To: André Draszik
Cc: Vinod Koul, Neil Armstrong, Peter Griffin, Tudor Ambarus,
Joy Chakraborty, Naveen Kumar, linux-phy, linux-kernel,
linux-arm-kernel, linux-samsung-soc, kernel test robot
In-Reply-To: <f13cf27da2d1d7d8a54448391c16a8eae349e8a5.camel@linaro.org>
On Wed, Jan 28, 2026 at 11:57 AM André Draszik <andre.draszik@linaro.org> wrote:
>
> Hi Roy,
>
> On Mon, 2026-01-26 at 18:34 -0800, Roy Luo wrote:
> > the wrong example. Let's look at this build error scenario with
> > "depends on TYPEC || COMPILE_TEST":
> > Given COMPILE_TEST=y and CONFIG_TYPEC=m, the
> > "depends on" would be evaluated as max(y,m) = y.
> > Hence, CONFIG_PHY_GOOGLE_USB is allowed to be
> > set to y, m or n. When it's set to y, the exact build error
> > would be introduced. I also tested this locally and saw
> > the build error.
>
> Thanks for trying Roy. In that case, it should be written as
>
> depends on TYPEC || (TYPEC=n && COMPILE_TEST)
>
>
> Cheers,
> Andre'
Hi Andre',
Thank you for the clarification. I noticed similar expressions
like "(CONFIG_X=n && COMPILE_TEST)" in other drivers,
though it seems less common, which is why I was initially hesitant.
I will send out v2 now with this change included.
Thanks,
Roy
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH next v2] phy: google: fix build dependency for Google Tensor USB PHY
From: Roy Luo @ 2026-01-28 21:22 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Peter Griffin, André Draszik,
Tudor Ambarus, Joy Chakraborty, Naveen Kumar
Cc: linux-phy, linux-kernel, linux-arm-kernel, linux-samsung-soc,
kernel test robot, Roy Luo
The Google Tensor USB PHY driver uses the Type-C switch framework to
handle orientation changes. However, the Kconfig did not specify a
dependency on the TYPEC framework, leading to undefined reference
errors when building for architectures or configurations where
CONFIG_TYPEC is configured as a module while CONFIG_PHY_GOOGLE_USB
is configured as built-in.
Add 'depends on TYPEC' to the PHY_GOOGLE_USB entry to ensure all
required symbols are available during linking, and 'COMPILE_TEST'
to expand test coverage.
Fixes: cbce66669c82 ("phy: Add Google Tensor SoC USB PHY driver")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202601210825.ELrpQeED-lkp@intel.com/
Reviewed-by: Peter Griffin <peter.griffin@linaro.org>
Signed-off-by: Roy Luo <royluo@google.com>
---
Changes in v2:
- Add '(TYPEC=n && COMPILE_TEST)' to build dependency to expand test
coverage as suggested by André Draszik <andre.draszik@linaro.org>
- Keep Peter's Reviewed-by tag because the change suggested by André
is a trivial improvement.
- Link to v1: https://lore.kernel.org/r/20260121-next-v1-1-c18068b091b9@google.com
---
drivers/phy/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 142e7b0ef2efb9209781800ee47b820a91b115ae..29953c911deda5fd3c43aef6da28d96954a22f8d 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -49,6 +49,7 @@ config GENERIC_PHY_MIPI_DPHY
config PHY_GOOGLE_USB
tristate "Google Tensor SoC USB PHY driver"
+ depends on TYPEC || (TYPEC=n && COMPILE_TEST)
select GENERIC_PHY
help
Enable support for the USB PHY on Google Tensor SoCs, starting with
---
base-commit: 8bb92fd7a04077925c8330f46a6ab44c80ca59f4
change-id: 20260121-next-b949189cacf4
Best regards,
--
Roy Luo <royluo@google.com>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* Re: [PATCH next v2] phy: google: fix build dependency for Google Tensor USB PHY
From: André Draszik @ 2026-01-28 22:34 UTC (permalink / raw)
To: Roy Luo, Vinod Koul, Neil Armstrong, Peter Griffin, Tudor Ambarus,
Joy Chakraborty, Naveen Kumar
Cc: linux-phy, linux-kernel, linux-arm-kernel, linux-samsung-soc,
kernel test robot
In-Reply-To: <20260128-next-v2-1-624bdae8e6d0@google.com>
On Wed, 2026-01-28 at 21:22 +0000, Roy Luo wrote:
> The Google Tensor USB PHY driver uses the Type-C switch framework to
> handle orientation changes. However, the Kconfig did not specify a
> dependency on the TYPEC framework, leading to undefined reference
> errors when building for architectures or configurations where
> CONFIG_TYPEC is configured as a module while CONFIG_PHY_GOOGLE_USB
> is configured as built-in.
>
> Add 'depends on TYPEC' to the PHY_GOOGLE_USB entry to ensure all
> required symbols are available during linking, and 'COMPILE_TEST'
> to expand test coverage.
>
> Fixes: cbce66669c82 ("phy: Add Google Tensor SoC USB PHY driver")
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202601210825.ELrpQeED-lkp@intel.com/
> Reviewed-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Roy Luo <royluo@google.com>
> ---
> Changes in v2:
> - Add '(TYPEC=n && COMPILE_TEST)' to build dependency to expand test
> coverage as suggested by André Draszik <andre.draszik@linaro.org>
> - Keep Peter's Reviewed-by tag because the change suggested by André
> is a trivial improvement.
> - Link to v1: https://lore.kernel.org/r/20260121-next-v1-1-c18068b091b9@google.com
> ---
> drivers/phy/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: André Draszik <andre.draszik@linaro.org>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH net-next v2 00/14] net: stmmac: SerDes, PCS, BASE-X, and inband goodies
From: Mohd Ayaan Anwar @ 2026-01-29 7:27 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Andrew Lunn, Heiner Kallweit, Alexandre Torgue, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Konrad Dybcio,
linux-arm-kernel, linux-arm-msm, linux-phy, linux-stm32,
Maxime Coquelin, Neil Armstrong, netdev, Paolo Abeni, Vinod Koul
In-Reply-To: <aXjdAZeUl2Dsu4mE@shell.armlinux.org.uk>
On Tue, Jan 27, 2026 at 03:42:57PM +0000, Russell King (Oracle) wrote:
> On Tue, Jan 27, 2026 at 08:27:50PM +0530, Mohd Ayaan Anwar wrote:
> > During pcs_init, BIT(8) of GMAC_AN_STATUS is 0:
> > [ 7.985913] [DBG] GMAC_AN_STATUS = 8
>
> Hmm. This means that your hardware doesn't support TBI or RTBI modes
> (which is what the dwmac core uses for BASE-X) and what it's actually
> offering is an up-clocked Cisco SGMII implementation.
>
> With AN disabled, this is compatible with 2500BASE-X implementations
> that do not require AN.
Yes, this hardware implements what some vendors call OCSGMII (i.e.
2500BASE-X without in-band signalling).
> > I also tried enabling comma detect during dwmac_integrated_pcs_config,
> > but I am still seeing the Tx timeouts. I remember that when I had
> > tested the patches in October (without the SerDes driver changes),
> > the link state used to flap, but the data path became functional
> > after the link stabilized.
>
> I wonder whether the SerDes needs to be calibrated after the link has
> come up and the clocks configured. phy_calibdate() will re-invoke the
> programming of the SerDes, so you could try adding that at the bottom
> of ethqos_configure_sgmii():
>
> return phy_calibrate(priv->plat->serdes);
>
> which will do the calibration after the clocks have been set, and see
> whether that stabilises the link.
Somehow booting up with a 2.5G link is more unstable after adding this.
Behaviour at 1G is pretty similar. I kept comma detection disabled
during these tests.
I was thinking about the "good" sequence (i.e., the current net-next
tree):
-> phylink_up
-> mac_link_up
-> fix_mac_speed
-> SerDes configured via phy_set_speed
-> stmmac_pcs_ctrl_ane(priv, enable/disable, 0)
Please let me know if you want me to try any other experiments.
Maybe I need to do some more testing after playing around with the
sequence followed by this series?
Ayaan
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH] phy: PHY_GOOGLE_USB should depend on ARCH_GOOGLE
From: Geert Uytterhoeven @ 2026-01-29 8:32 UTC (permalink / raw)
To: André Draszik
Cc: Geert Uytterhoeven, Vinod Koul, Neil Armstrong, Peter Griffin,
Tudor Ambarus, Naveen Kumar, Roy Luo, Joy Chakraborty,
Arnd Bergmann, linux-phy, linux-arm-kernel, linux-samsung-soc,
linux-kernel
In-Reply-To: <e955868c06547ac62d34d594d45dd07aa19e361b.camel@linaro.org>
Hi André,
On Wed, 28 Jan 2026 at 21:03, André Draszik <andre.draszik@linaro.org> wrote:
> On Tue, 2026-01-27 at 20:12 +0100, Geert Uytterhoeven wrote:
> > The Google Tensor SoC USB PHY is only present on Google Tensor G5
> > (Laguna) SoCs. Hence add a dependency on ARCH_GOOGLE, to prevent asking
> > the user about this driver when configuring a kernel without Google
> > Tensor SoC support.
> >
> > Fixes: cbce66669c82ee9a ("phy: Add Google Tensor SoC USB PHY driver")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > ARCH_GOOGLE is not yet upstream, but that doesn't hurt.
>
> I can see your point, and have no objection, but for context, Greg
> had asked to not add dependencies on a config option that might or
> might not get added in the future - ARCH_GOOGLE:
The exact name may indeed still be under discussion. But we all know
there _will_ be a config option (unless the platform is abandoned before
upstreaming, and this driver will be removed again anyway ;-).
> Maybe an alternative would be to make it default=n, and change that
> default once ARCH_GOOGLE gets added?
It already defaults to disabled, as "default=n" is the default.
> > --- a/drivers/phy/Kconfig
> > +++ b/drivers/phy/Kconfig
> > @@ -49,6 +49,7 @@ config GENERIC_PHY_MIPI_DPHY
> >
> > config PHY_GOOGLE_USB
> > tristate "Google Tensor SoC USB PHY driver"
> > + depends on ARCH_GOOGLE || COMPILE_TEST
> > select GENERIC_PHY
> > help
> > Enable support for the USB PHY on Google Tensor SoCs, starting with
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy
From: Simon Horman @ 2026-01-29 9:54 UTC (permalink / raw)
To: Vincent Guittot
Cc: vkoul, neil.armstrong, krzk+dt, conor+dt, ciprianmarian.costea,
s32, p.zabel, linux, ghennadi.procopciuc, bogdan-gabriel.roman,
Ionut.Vicovan, alexandru-catalin.ionita, linux-phy, devicetree,
linux-kernel, linux-arm-kernel, netdev, Frank.li
In-Reply-To: <20260126092159.815968-3-vincent.guittot@linaro.org>
On Mon, Jan 26, 2026 at 10:21:57AM +0100, Vincent Guittot wrote:
...
> diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> new file mode 100644
> index 000000000000..8336c868c8dc
> --- /dev/null
> +++ b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> @@ -0,0 +1,569 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * SerDes driver for S32G SoCs
> + *
> + * Copyright 2021-2026 NXP
> + */
> +
> +#include <dt-bindings/phy/phy.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
Hi Vincent, all,
I think that you also need:
#include <linux/iopoll.h>
So that read_poll_timeout() is declared.
Else this patch causes a transient build failure
(for x86_64 allmodconfig)
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/processor.h>
> +#include <linux/reset.h>
> +#include <linux/units.h>
...
> +static int s32g_serdes_phy_set_mode_ext(struct phy *p,
> + enum phy_mode mode, int submode)
> +{
> + struct s32g_serdes *serdes = phy_get_drvdata(p);
> +
> + if (mode == PHY_MODE_PCIE)
> + return -EINVAL;
This is part of an AI Generated review.
I have looked over it and I think it warrants investigation.
For information on how to reproduce locally, as I did, please see [1].
[1] https://netdev-ai.bots.linux.dev/ai-local.html
This returns error if mode IS PHY_MODE_PCIE, but this is a PCIe PHY!
This looks like a typo. Should be !=.
> +
> + if (!is_pcie_phy_mode_valid(submode))
> + return -EINVAL;
> +
> + /*
> + * Do not configure SRIS or CRSS PHY MODE in conjunction
> + * with any SGMII mode on the same SerDes subsystem
> + */
> + if ((submode == CRSS || submode == SRIS) &&
> + serdes->ctrl.ss_mode != 0)
> + return -EINVAL;
> +
> + /*
> + * Internal reference clock cannot be used with either Common clock
> + * or Spread spectrum, leaving only SRNSS
> + */
> + if (submode != SRNS && !serdes->ctrl.ext_clk)
> + return -EINVAL;
> +
> + serdes->pcie.phy_mode = submode;
The AI review also suggested that it may be unsafe
to set the submode after s32g_serdes_phy_power_on()
has been called. And that there is nothing preventing that.
TBH, I am unsure if either of those statements are true.
But it seems worth validating with you.
> +
> + return 0;
> +}
...
> +static int s32g_serdes_get_ctrl_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
> +{
> + struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
> + struct device *dev = &pdev->dev;
> + int ret, idx;
> +
> + ret = of_property_read_u32(dev->of_node, "nxp,sys-mode",
> + &ctrl->ss_mode);
> + if (ret) {
> + dev_err(dev, "Failed to get SerDes subsystem mode\n");
> + return -EINVAL;
> + }
> +
> + if (ctrl->ss_mode > S32G_SERDES_MODE_MAX) {
> + dev_err(dev, "Invalid SerDes subsystem mode %u\n",
> + ctrl->ss_mode);
> + return -EINVAL;
> + }
> +
> + ctrl->ss_base = devm_platform_ioremap_resource_byname(pdev, "ss_pcie");
> + if (IS_ERR(ctrl->ss_base)) {
> + dev_err(dev, "Failed to map 'ss_pcie'\n");
> + return PTR_ERR(ctrl->ss_base);
> + }
> +
> + ctrl->rst = devm_reset_control_get(dev, "serdes");
> + if (IS_ERR(ctrl->rst))
> + return dev_err_probe(dev, PTR_ERR(ctrl->rst),
> + "Failed to get 'serdes' reset control\n");
> +
> + ctrl->nclks = devm_clk_bulk_get_all(dev, &ctrl->clks);
> + if (ctrl->nclks < 1)
> + return dev_err_probe(dev, ctrl->nclks,
> + "Failed to get SerDes clocks\n");
If devm_clk_bulk_get_all returns 0 then this value will
be passed to dev_err_probe(). And 0 will, in turn be returned by
dev_err_probe() and this function. However, that will be treated
as success by the caller, even though this is an error condition.
Perhaps something like this is more appropriate if ctrl->nclks
must be greater than 0. (Completely untested!)
if (ctrl->nclks < 1) {
ret = ctrl->nclks ? : -EINVAL;
return dev_err_probe(dev, ret,
"Failed to get SerDes clocks\n");
}
Flagged by Smatch.
...
> +static int s32g_serdes_parse_lanes(struct device *dev, struct s32g_serdes *serdes)
> +{
> + int ret;
> +
> + for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> + ret = s32g2_serdes_create_phy(serdes, of_port);
> + if (ret)
> + break;
> + }
> +
> + return ret;
Perhaps it cannot occur.
But if the loop above iterates zero times,
then ret will be used uninitialised here.
Also flagged by Smatch.
> +}
> +
> +static int s32g_serdes_probe(struct platform_device *pdev)
> +{
> + struct s32g_serdes *serdes;
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + serdes = devm_kzalloc(dev, sizeof(*serdes), GFP_KERNEL);
> + if (!serdes)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, serdes);
> + serdes->dev = dev;
> +
> + ret = s32g_serdes_get_ctrl_resources(pdev, serdes);
> + if (ret)
> + return ret;
> +
> + ret = s32g_serdes_get_pcie_resources(pdev, serdes);
> + if (ret)
> + return ret;
> +
> + ret = s32g_serdes_parse_lanes(dev, serdes);
> + if (ret)
> + return ret;
The I review also says:
The probe function calls s32g_serdes_init() which enables clocks,
configures hardware, and deasserts reset. However,
s32g_serdes_parse_lanes() creates PHY providers via
devm_of_phy_provider_register().
Problem: PHY consumers can start calling PHY ops (like power_on) as soon
as the provider is registered, but the hardware isn't initialized until
s32g_serdes_init() runs afterward. This creates a race window.
Recommendation: Move s32g_serdes_init() before s32g_serdes_parse_lanes().
> +
> + ret = s32g_serdes_init(serdes);
> +
> + return ret;
nit: This could be more succinctly written as:
return s32g_serdes_init(serdes);
> +}
...
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy
From: Russell King (Oracle) @ 2026-01-29 11:17 UTC (permalink / raw)
To: Vincent Guittot
Cc: vkoul, neil.armstrong, krzk+dt, conor+dt, ciprianmarian.costea,
s32, p.zabel, ghennadi.procopciuc, bogdan-gabriel.roman,
Ionut.Vicovan, alexandru-catalin.ionita, linux-phy, devicetree,
linux-kernel, linux-arm-kernel, netdev, Frank.li
In-Reply-To: <20260126092159.815968-3-vincent.guittot@linaro.org>
On Mon, Jan 26, 2026 at 10:21:57AM +0100, Vincent Guittot wrote:
> +/*
> + * Until now, there is no generic way to describe and set PCIe clock mode.
> + * PCIe controller uses the default CRNS = 0 mode.
> + */
> +enum pcie_phy_mode {
> + CRNS = 0, /* Common Reference Clock, No Spread Spectrum */
> + CRSS = 1, /* Common Reference Clock, Spread Spectrum */
> + SRNS = 2, /* Separate Reference Clock, No Spread Spectrum */
> + SRIS = 3 /* Separate Reference Clock, Spread Spectrum */
> +};
So this is a PCIe thing. If it's part of the driver's API, then it
should be common and not driver-private.
> +static inline bool is_pcie_phy_mode_valid(int mode)
> +{
> + switch (mode) {
> + case CRNS:
> + case CRSS:
> + case SRNS:
> + case SRIS:
> + return true;
> + default:
> + return false;
> + }
> +}
This checks that the submode is one of the PCIe private modes that this
driver wants to see.
> +
> +static int s32g_serdes_phy_set_mode_ext(struct phy *p,
> + enum phy_mode mode, int submode)
> +{
> + struct s32g_serdes *serdes = phy_get_drvdata(p);
> +
> + if (mode == PHY_MODE_PCIE)
> + return -EINVAL;
> +
> + if (!is_pcie_phy_mode_valid(submode))
> + return -EINVAL;
This checks for the PCIe submode, but notice the test immediately
above. PCIE mode is being rejected. So, this driver supports
everything else but PCIe.
That doesn't seem right.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 3/4] phy: s32g: Add serdes xpcs subsystem
From: Simon Horman @ 2026-01-29 11:59 UTC (permalink / raw)
To: Vincent Guittot
Cc: vkoul, neil.armstrong, krzk+dt, conor+dt, ciprianmarian.costea,
s32, p.zabel, linux, ghennadi.procopciuc, bogdan-gabriel.roman,
Ionut.Vicovan, alexandru-catalin.ionita, linux-phy, devicetree,
linux-kernel, linux-arm-kernel, netdev, Frank.li
In-Reply-To: <20260126092159.815968-4-vincent.guittot@linaro.org>
On Mon, Jan 26, 2026 at 10:21:58AM +0100, Vincent Guittot wrote:
...
> diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
...
> +static void s32g_serdes_prepare_pma_mode5(struct s32g_serdes *serdes)
> +{
> + u32 val;
> + /* Configure TX_VBOOST_LVL and TX_TERM_CTRL */
> + val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_2);
> + val &= ~(EXT_TX_VBOOST_LVL_MASK | EXT_TX_TERM_CTRL_MASK);
> + val |= FIELD_PREP(EXT_TX_VBOOST_LVL_MASK, 0x3) |
> + FIELD_PREP(EXT_TX_TERM_CTRL_MASK, 0x4);
> + writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_2);
This is part of an AI Generated review.
I have looked over it and I think it warrants investigation.
For information on how to reproduce locally, as I did, please see [1].
The entire s32g_serdes_prepare_pma_mode5() function is ~70 lines of
magic numbers with zero explanation. These appear to be
hardware-specific PLL/PHY tuning parameters for 2.5G mode.
Please consider using #defines, to give values names.
...
> +static int s32g_serdes_init_clks(struct s32g_serdes *serdes)
> +{
> + struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
> + struct s32g_xpcs_ctrl *xpcs = &serdes->xpcs;
> + int ret, order[2], xpcs_id;
> + size_t i;
> +
> + switch (ctrl->ss_mode) {
> + case 0:
> + return 0;
> + case 1:
> + order[0] = 0;
> + order[1] = XPCS_DISABLED;
> + break;
> + case 2:
> + case 5:
> + order[0] = 1;
> + order[1] = XPCS_DISABLED;
> + break;
> + case 3:
> + order[0] = 1;
> + order[1] = 0;
> + break;
> + case 4:
> + order[0] = 0;
> + order[1] = 1;
> + break;
> + default:
> + return -EINVAL;
AI review also flags that s32g_serdes_get_ctrl_resources() ensures that
ss_mode is <= 5. So this check is unnecessary.
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(order); i++) {
> + xpcs_id = order[i];
> +
> + if (xpcs_id == XPCS_DISABLED)
> + continue;
> +
> + ret = s32g_xpcs_init_plls(xpcs->phys[xpcs_id]);
> + if (ret)
> + return ret;
> + }
> +
> + if (ctrl->ss_mode == 5) {
> + s32g_serdes_prepare_pma_mode5(serdes);
> +
> + ret = s32g_xpcs_pre_pcie_2g5(xpcs->phys[1]);
Also from the AI review:
In mode 5, code directly accesses xpcs->phys[1] without checking if
it was created. If XPCS1 wasn't created for some reason (DT
misconfiguration, probe failure), this NULL dereferences.
...
> @@ -460,6 +741,41 @@ static int s32g2_serdes_create_phy(struct s32g_serdes *serdes, struct device_nod
...
> + xpcs = devm_kmalloc(dev, sizeof(*xpcs), GFP_KERNEL);
> + if (IS_ERR(xpcs)) {
> + dev_err(dev, "Failed to allocate xpcs\n");
> + return -ENOMEM;
> + }
AI review also flags:
devm_kmalloc() returns NULL on failure, not an error pointer.
This check will never trigger.
[1] https://netdev-ai.bots.linux.dev/ai-local.html
> +
> + xpcs_ctrl->phys[port] = xpcs;
> +
> + xpcs->an = of_property_read_bool(dev->of_node, "nxp,xpcs_an");
AI review flags that nxp,xpcs_an is not part of the binding.
...
> +struct phylink_pcs *s32g_serdes_pcs_create(struct device *dev, struct device_node *np)
> +{
> + struct platform_device *pdev;
> + struct device_node *pcs_np;
> + struct s32g_serdes *serdes;
> + u32 port;
> +
> + if (of_property_read_u32(np, "reg", &port))
> + return ERR_PTR(-EINVAL);
> +
> + if (port > S32G_SERDES_XPCS_MAX)
> + return ERR_PTR(-EINVAL);
> +
> + /* The PCS pdev is attached to the parent node */
> + pcs_np = of_get_parent(np);
> + if (!pcs_np)
> + return ERR_PTR(-ENODEV);
> +
> + if (!of_device_is_available(pcs_np)) {
> + of_node_put(pcs_np);
> + return ERR_PTR(-ENODEV);
> + }
> +
> + pdev = of_find_device_by_node(pcs_np);
> + of_node_put(pcs_np);
> + if (!pdev || !platform_get_drvdata(pdev)) {
> + if (pdev)
> + put_device(&pdev->dev);
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> + serdes = platform_get_drvdata(pdev);
Also from the AI review:
On success, the function gets a reference to pdev but never
releases it. This leaks a device reference every time a MAC driver
calls s32g_serdes_pcs_create().
> +
> + return &serdes->xpcs.phys[port]->pcs;
Also from the AI review:
The check port > S32G_SERDES_XPCS_MAX allows port=2,
but array only has indices 0 and 1.
Also I'm not seeing bounds checking on port in s32g2_serdes_create_phy
which uses port as an index for the same array both directly and indirectly
via s32g_serdes_xpcs_init().
...
> diff --git a/drivers/phy/freescale/phy-nxp-s32g-xpcs.c b/drivers/phy/freescale/phy-nxp-s32g-xpcs.c
...
> +static int s32g_xpcs_regmap_reg_read(void *context, unsigned int reg,
> + unsigned int *result)
> +{
> + struct s32g_xpcs *xpcs = context;
> + u16 ofsleft = (reg >> 8) & 0xffffU;
> + u16 ofsright = (reg & 0xffU);
> +
> + writew(ofsleft, xpcs->base + ADDR1_OFS);
> + *result = readw(xpcs->base + (ofsright * 4));
> +
> + return 0;
> +}
> +
> +static int s32g_xpcs_regmap_reg_write(void *context, unsigned int reg,
> + unsigned int val)
> +{
> + struct s32g_xpcs *xpcs = context;
> + u16 ofsleft = (reg >> 8) & 0xffffU;
> + u16 ofsright = (reg & 0xffU);
> +
> + writew(ofsleft, xpcs->base + ADDR1_OFS);
> + writew(val, xpcs->base + (ofsright * 4));
> +
> + return 0;
> +}
> +
> +static const struct regmap_config s32g_xpcs_regmap_config = {
> + .reg_bits = 16,
> + .val_bits = 16,
> + .reg_read = s32g_xpcs_regmap_reg_read,
> + .reg_write = s32g_xpcs_regmap_reg_write,
> + .wr_table = &s32g_xpcs_wr_table,
> + .rd_table = &s32g_xpcs_rd_table,
> + .max_register = 0x1F80E1,
> +};
AI review also flags that s32g_xpcs_regmap_reg_read and
s32g_xpcs_regmap_reg_write do not protect against concurrent access.
...
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 4/4] MAINTAINERS: Add MAINTAINER for NXP S32G Serdes driver
From: Simon Horman @ 2026-01-29 12:07 UTC (permalink / raw)
To: Vincent Guittot
Cc: vkoul, neil.armstrong, krzk+dt, conor+dt, ciprianmarian.costea,
s32, p.zabel, linux, ghennadi.procopciuc, bogdan-gabriel.roman,
Ionut.Vicovan, alexandru-catalin.ionita, linux-phy, devicetree,
linux-kernel, linux-arm-kernel, netdev, Frank.li
In-Reply-To: <20260126092159.815968-5-vincent.guittot@linaro.org>
On Mon, Jan 26, 2026 at 10:21:59AM +0100, Vincent Guittot wrote:
> Add a new entry for S32G Serdes driver.
>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
> MAINTAINERS | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 765ad2daa218..888674a308a5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3202,6 +3202,15 @@ S: Maintained
> F: Documentation/devicetree/bindings/net/nxp,s32-dwmac.yaml
> F: drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
>
> +ARM/NXP S32G SERDES DRIVER
> +M: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> +R: NXP S32 Linux Team <s32@nxp.com>
> +L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> +S: Maintained
> +F: Documentation/devicetree/bindings/pci/nxp,s32g-serdes.yaml
This patchset adds the following file, not the one above:
Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
^^^
> +F: drivers/phy/freescale/phy-nxp-s32g-*
> +F: include/linux/pcs/pcs-nxp-xpcs.h
> +
> ARM/Orion SoC/Technologic Systems TS-78xx platform support
> M: Alexander Clouter <alex@digriz.org.uk>
> L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> --
> 2.43.0
>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 3/4] phy: s32g: Add serdes xpcs subsystem
From: Russell King (Oracle) @ 2026-01-29 12:30 UTC (permalink / raw)
To: Vincent Guittot
Cc: vkoul, neil.armstrong, krzk+dt, conor+dt, ciprianmarian.costea,
s32, p.zabel, ghennadi.procopciuc, bogdan-gabriel.roman,
Ionut.Vicovan, alexandru-catalin.ionita, linux-phy, devicetree,
linux-kernel, linux-arm-kernel, netdev, Frank.li
In-Reply-To: <20260126092159.815968-4-vincent.guittot@linaro.org>
On Mon, Jan 26, 2026 at 10:21:58AM +0100, Vincent Guittot wrote:
> s32g SoC family includes 2 serdes subsystems which are made of one PCIe
> controller, 2 XPCS and one Phy. The Phy got 2 lanes that can be configure
> to output PCIe lanes and/or SGMII.
>
> Add XPCS and SGMII support.
>
> Co-developed-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> Co-developed-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> Signed-off-by: Alexandru-Catalin Ionita <alexandru-catalin.ionita@nxp.com>
> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Co-developed-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
> Signed-off-by: Ionut Vicovan <Ionut.Vicovan@nxp.com>
> Co-developed-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
> Signed-off-by: Bogdan Roman <bogdan-gabriel.roman@nxp.com>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
I'm not doing a full review for this patch yet.
> +/*
> + * Note: This function should be compatible with phylink.
> + * That means it should only modify link, duplex, speed
> + * an_complete, pause.
> + */
> +static int s32g_xpcs_get_state(struct s32g_xpcs *xpcs,
> + struct phylink_link_state *state)
> +{
> + struct device *dev = xpcs->dev;
> + unsigned int mii_ctrl, val, ss;
> + bool ss6, ss13, an_enabled, intr_en;
> +
> + mii_ctrl = s32g_xpcs_read(xpcs, SR_MII_CTRL);
> + an_enabled = !!(mii_ctrl & AN_ENABLE);
> + intr_en = !!(s32g_xpcs_read(xpcs, VR_MII_AN_CTRL) & MII_AN_INTR_EN);
> +
> + /* Check this important condition */
> + if (an_enabled && !intr_en) {
> + dev_err(dev, "Invalid SGMII AN config interrupt is disabled\n");
> + return -EINVAL;
> + }
This isn't an interrupt handler. Phylink calls it from the state
resolver, which _may_ be triggered by an interrupt handler, but will
also be called at other times.
> +
> + if (an_enabled) {
> + /* MLO_AN_INBAND */
> + state->speed = SPEED_UNKNOWN;
> + state->link = 0;
> + state->duplex = DUPLEX_UNKNOWN;
> + state->an_complete = 0;
> + state->pause = MLO_PAUSE_NONE;
Have you looked at the initial state that phylink sets up before
calling the .pcs_get_state() method? See phylink_mac_pcs_get_state().
speed/duplex/pause/an_complete are already setup like that for you if
autoneg is enabled. link is the only member you'd need to touch.
> + val = s32g_xpcs_read(xpcs, VR_MII_AN_INTR_STS);
> +
> + /* Interrupt is raised with each SGMII AN that is in cases
> + * Link down - Every SGMII link timer expire
> + * Link up - Once before link goes up
> + * So either linkup or raised interrupt mean AN was completed
> + */
> + if ((val & CL37_ANCMPLT_INTR) || (val & CL37_ANSGM_STS_LINK)) {
> + state->an_complete = 1;
> + if (val & CL37_ANSGM_STS_LINK)
> + state->link = 1;
> + else
> + return 0;
> + if (val & CL37_ANSGM_STS_DUPLEX)
> + state->duplex = DUPLEX_FULL;
> + else
> + state->duplex = DUPLEX_HALF;
> + ss = FIELD_GET(CL37_ANSGM_STS_SPEED_MASK, val);
> + } else {
> + return 0;
> + }
> +
> + } else {
> + /* MLO_AN_FIXED, MLO_AN_PHY */
This function won't be called in those modes, so this is a misleading
comment. It can be called in MLO_AN_INBAND but when autoneg is disabled.
> + val = s32g_xpcs_read(xpcs, SR_MII_STS);
> + state->link = !!(val & LINK_STS);
> + state->an_complete = 0;
> + state->pause = MLO_PAUSE_NONE;
> +
> + if (mii_ctrl & DUPLEX_MODE)
> + state->duplex = DUPLEX_FULL;
> + else
> + state->duplex = DUPLEX_HALF;
> +
> + /*
> + * Build similar value as CL37_ANSGM_STS_SPEED with
> + * SS6 and SS13 of SR_MII_CTRL:
> + * - 0 for 10 Mbps
> + * - 1 for 100 Mbps
> + * - 2 for 1000 Mbps
> + */
> + ss6 = !!(mii_ctrl & SS6);
> + ss13 = !!(mii_ctrl & SS13);
> + ss = ss6 << 1 | ss13;
> + }
> +
> + switch (ss) {
> + case CL37_ANSGM_10MBPS:
> + state->speed = SPEED_10;
> + break;
> + case CL37_ANSGM_100MBPS:
> + state->speed = SPEED_100;
> + break;
> + case CL37_ANSGM_1000MBPS:
> + state->speed = SPEED_1000;
> + break;
> + default:
> + dev_err(dev, "Failed to interpret the value of SR_MII_CTRL\n");
> + break;
> + }
> +
> + val = s32g_xpcs_read(xpcs, VR_MII_DIG_CTRL1);
> + if ((val & EN_2_5G_MODE) && state->speed == SPEED_1000)
> + state->speed = SPEED_2500;
> +
> + /* Cover SGMII AN inability to distigunish between 1G and 2.5G */
> + if ((val & EN_2_5G_MODE) &&
> + state->speed != SPEED_2500 && an_enabled) {
> + dev_err(dev, "Speed not supported in SGMII AN mode\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int s32g_xpcs_config_an(struct s32g_xpcs *xpcs,
> + const struct phylink_link_state state)
> +{
> + bool an_enabled = false;
> +
> + an_enabled = linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> + state.advertising);
> + if (!an_enabled)
> + return 0;
Don't check the autoneg bit. This is the media-side autoneg, not
the PCS autoneg.
> +
> + s32g_xpcs_write_bits(xpcs, VR_MII_DIG_CTRL1,
> + CL37_TMR_OVRRIDE, CL37_TMR_OVRRIDE);
> +
> + s32g_xpcs_write_bits(xpcs, VR_MII_AN_CTRL,
> + PCS_MODE_MASK | MII_AN_INTR_EN,
> + FIELD_PREP(PCS_MODE_MASK, PCS_MODE_SGMII) | MII_AN_INTR_EN);
> + /* Enable SGMII AN */
> + s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, AN_ENABLE, AN_ENABLE);
> + /* Enable SGMII AUTO SW */
> + s32g_xpcs_write_bits(xpcs, VR_MII_DIG_CTRL1,
> + MAC_AUTO_SW, MAC_AUTO_SW);
> +
> + return 0;
> +}
> +
> +static int s32g_xpcs_config(struct s32g_xpcs *xpcs,
> + const struct phylink_link_state state)
> +{
> + struct device *dev = xpcs->dev;
> + unsigned int val = 0, duplex = 0;
> + int ret = 0;
> + int speed = state.speed;
> + bool an_enabled;
> +
> + /* Configure adaptive MII width */
> + s32g_xpcs_write_bits(xpcs, VR_MII_AN_CTRL, MII_CTRL, 0);
> +
> + an_enabled = !!(s32g_xpcs_read(xpcs, SR_MII_CTRL) & AN_ENABLE);
> +
> + dev_dbg(dev, "xpcs_%d: speed=%u duplex=%d an=%d\n", xpcs->id,
> + speed, state.duplex, an_enabled);
> +
> + if (an_enabled) {
> + switch (speed) {
> + case SPEED_10:
> + case SPEED_100:
> + case SPEED_1000:
> + s32g_xpcs_write(xpcs, VR_MII_LINK_TIMER_CTRL, 0x2faf);
> + break;
> + case SPEED_2500:
> + s32g_xpcs_write(xpcs, VR_MII_LINK_TIMER_CTRL, 0x7a1);
> + s32g_xpcs_write_bits(xpcs, VR_MII_DIG_CTRL1, MAC_AUTO_SW, 0);
Configuring the link timer _after_ the link has already come up looks
completely wrong to me... this should be done when .pcs_config() detects
that the PHY interface mode has changed.
> + break;
> + default:
> + dev_err(dev, "Speed not recognized. Can't setup xpcs\n");
> + return -EINVAL;
> + }
> +
> + s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, RESTART_AN, RESTART_AN);
As this is called from the .pcs_link_up() method, expect the link to
go bouncey bouncy bouncy if you restart AN _after_ the link has
come up.
> +
> + ret = s32g_xpcs_wait_an_done(xpcs);
> + if (ret)
> + dev_warn(dev, "AN did not finish for XPCS%d", xpcs->id);
> +
> + /* Clear the AN CMPL intr */
> + s32g_xpcs_write_bits(xpcs, VR_MII_AN_INTR_STS, CL37_ANCMPLT_INTR, 0);
> + } else {
> + s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, AN_ENABLE, 0);
> + s32g_xpcs_write_bits(xpcs, VR_MII_AN_CTRL, MII_AN_INTR_EN, 0);
> +
> + switch (speed) {
> + case SPEED_10:
> + break;
> + case SPEED_100:
> + val = SS13;
> + break;
> + case SPEED_1000:
> + val = SS6;
> + break;
> + case SPEED_2500:
> + val = SS6;
> + break;
> + default:
> + dev_err(dev, "Speed not supported\n");
> + break;
> + }
> +
> + if (state.duplex == DUPLEX_FULL)
> + duplex = DUPLEX_MODE;
> +
> + s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, DUPLEX_MODE, duplex);
> +
> + if (speed == SPEED_2500) {
> + ret = s32g_serdes_bifurcation_pll_transit(xpcs, XPCS_PLLB);
> + if (ret)
> + dev_err(dev, "Switch to PLLB failed\n");
> + } else {
> + ret = s32g_serdes_bifurcation_pll_transit(xpcs, XPCS_PLLA);
> + if (ret)
> + dev_err(dev, "Switch to PLLA failed\n");
> + }
> +
> + s32g_xpcs_write_bits(xpcs, SR_MII_CTRL, SS6 | SS13, val);
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * phylink_pcs_ops fops
They are not "fops" which commonly refers to struct file_operations
> + */
> +
> +static void s32cc_phylink_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
> + struct phylink_link_state *state)
> +{
> + struct s32g_xpcs *xpcs = phylink_pcs_to_s32g_xpcs(pcs);
> +
> + s32g_xpcs_get_state(xpcs, state);
> +}
Seems to me a pointless wrapper.
> +
> +static int s32cc_phylink_pcs_config(struct phylink_pcs *pcs,
> + unsigned int neg_mode,
> + phy_interface_t interface,
> + const unsigned long *advertising,
> + bool permit_pause_to_mac)
> +{
> + struct s32g_xpcs *xpcs = phylink_pcs_to_s32g_xpcs(pcs);
> + struct phylink_link_state state = { 0 };
> +
> + if (!(neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED))
> + return 0;
> +
> + linkmode_copy(state.advertising, advertising);
> +
> + return s32g_xpcs_config_an(xpcs, state);
Given this is the only callsite for this function, and the only thing
you pass is the advertising mask, why pass a struct phylink_link_state
rather than the advertising mask?
> +}
> +
> +static void s32cc_phylink_pcs_restart_an(struct phylink_pcs *pcs)
> +{
> + /* Not yet */
> +}
> +
> +static void s32cc_phylink_pcs_link_up(struct phylink_pcs *pcs,
> + unsigned int neg_mode,
> + phy_interface_t interface, int speed,
> + int duplex)
> +{
> + struct s32g_xpcs *xpcs = phylink_pcs_to_s32g_xpcs(pcs);
> + struct phylink_link_state state = { 0 };
> +
> + state.speed = speed;
> + state.duplex = duplex;
> + state.an_complete = false;
an_complete is never an "input" to drivers. It's a state from PCS
drivers back to phylink. Also, s32g_xpcs_config never looks at this.
> +
> + s32g_xpcs_config(xpcs, state);
Again, the only things that this function uses are the speed and
duplex, so why wrap them up into a struct?
> +}
> +
> +static const struct phylink_pcs_ops s32cc_phylink_pcs_ops = {
> + .pcs_get_state = s32cc_phylink_pcs_get_state,
> + .pcs_config = s32cc_phylink_pcs_config,
> + .pcs_an_restart = s32cc_phylink_pcs_restart_an,
> + .pcs_link_up = s32cc_phylink_pcs_link_up,
> +};
Please implement .pcs_inband_caps. As you don't support disabling
inband for SGMII, that means you can't support MLO_AN_PHY mode
reliably.
Also note that there are PHYs out there that do _not_ provide SGMII
inband, which means if you have it enabled, and you're relying on it
to complete, you won't be able to interface with those PHYs. There's
such a PHY on a SFP module.
If this driver is purely for a network PCS, then please locate it in
drivers/net/pcs.
I'm pretty sure there's other stuff I've missed as far as the phylink
API goes, so please expect further review once you've addressed the
comments above.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 0/4] Serdes: s32g: Add support for serdes subsystem
From: Russell King (Oracle) @ 2026-01-29 12:36 UTC (permalink / raw)
To: Vincent Guittot
Cc: vkoul, neil.armstrong, krzk+dt, conor+dt, ciprianmarian.costea,
s32, p.zabel, ghennadi.procopciuc, Ionut.Vicovan, linux-phy,
devicetree, linux-kernel, linux-arm-kernel, netdev, Frank.li
In-Reply-To: <20260126092159.815968-1-vincent.guittot@linaro.org>
Please drop these addresses from future patch series:
alexandru-catalin.ionita@nxp.com
host nxp-com.mail.protection.outlook.com [2a01:111:f403:ca09::7]
SMTP error from remote mail server after RCPT TO:<alexandru-catalin.ionita@n
xp.com>:
550 5.4.1 Recipient address rejected: Access denied. For more information se
e https://aka.ms/EXOSmtpErrors [AM4PEPF00027A62.eurprd04.prod.outlook.com 2026-0
1-29T12:31:01.197Z 08DE5971FB66165F]
bogdan-gabriel.roman@nxp.com
host nxp-com.mail.protection.outlook.com [2a01:111:f403:ca09::6]
SMTP error from remote mail server after RCPT TO:<bogdan-gabriel.roman@nxp.c
om>:
550 5.4.1 Recipient address rejected: Access denied. For more information se
e https://aka.ms/EXOSmtpErrors [AM3PEPF0000A798.eurprd04.prod.outlook.com 2026-0
1-29T12:31:00.781Z 08DE596525364766]
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: serdes: s32g: Add NXP serdes subsystem
From: Russell King (Oracle) @ 2026-01-29 12:50 UTC (permalink / raw)
To: Vincent Guittot
Cc: vkoul, neil.armstrong, krzk+dt, conor+dt, ciprianmarian.costea,
s32, p.zabel, ghennadi.procopciuc, bogdan-gabriel.roman,
Ionut.Vicovan, alexandru-catalin.ionita, linux-phy, devicetree,
linux-kernel, linux-arm-kernel, netdev, Frank.li
In-Reply-To: <20260126092159.815968-2-vincent.guittot@linaro.org>
On Mon, Jan 26, 2026 at 10:21:56AM +0100, Vincent Guittot wrote:
> Describe the serdes subsystem available on the S32G platforms.
>
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
> .../bindings/phy/nxp,s32g-serdes.yaml | 154 ++++++++++++++++++
> 1 file changed, 154 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
>
> diff --git a/Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml b/Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
> new file mode 100644
> index 000000000000..fad34bee2a4f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/nxp,s32g-serdes.yaml
> @@ -0,0 +1,154 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/nxp,s32g-serdes.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP S32G2xxx/S32G3xxx SerDes PHY subsystem
> +
> +maintainers:
> + - Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> +
> +description: |
> + The SerDes subsystem on S32G SoC Family includes two types of PHYs:
> + - One PCIe PHY: Supports various PCIe operation modes
> + - Two Ethernet Physical Coding Sublayer (XPCS) controllers
> +
> + SerDes operation mode selects the enabled PHYs and speeds. Clock frequency
> + must be adapted accordingly. Below table describes all possible operation
> + modes.
> +
> + Mode PCIe XPCS0 XPCS1 PHY clock Description
> + SGMII SGMII (MHz)
> + -------------------------------------------------------------------------
> + 0 Gen3 N/A N/A 100 Single PCIe
> + 1 Gen2 1.25Gbps N/A 100 PCIe/SGMII
> + 2 Gen2 N/A 1.25Gbps 100 PCIe/SGMII
> + 3 N/A 1.25Gbps 1.25Gbps 100,125 SGMII
> + 4 N/A 3.125/1.25Gbps 3.125/1.25Gbps 125 SGMII
> + 5 Gen2 N/A 3.125Gbps 100 PCIe/SGMII
Shouldn't the mode be configured via phy_set_mode_ext()?
This identifies whether it is operating as PCIe or for networking and
in the case of networking, the PHY interface mode should be passed as
the submode.
Have a look at include/linux/phy/pcie.h to see the submodes that may
be appropriate to pass to phy_set_mode_ext() - but talk to the PHY
subsystem maintainers.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy
From: Vincent Guittot @ 2026-01-29 13:01 UTC (permalink / raw)
To: Simon Horman
Cc: vkoul, neil.armstrong, krzk+dt, conor+dt, ciprianmarian.costea,
s32, p.zabel, linux, ghennadi.procopciuc, bogdan-gabriel.roman,
Ionut.Vicovan, alexandru-catalin.ionita, linux-phy, devicetree,
linux-kernel, linux-arm-kernel, netdev, Frank.li
In-Reply-To: <aXsuRTZUUnw0kdzV@horms.kernel.org>
On Thu, 29 Jan 2026 at 10:54, Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Jan 26, 2026 at 10:21:57AM +0100, Vincent Guittot wrote:
>
> ...
>
> > diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> > new file mode 100644
> > index 000000000000..8336c868c8dc
> > --- /dev/null
> > +++ b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> > @@ -0,0 +1,569 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
> > + * SerDes driver for S32G SoCs
> > + *
> > + * Copyright 2021-2026 NXP
> > + */
> > +
> > +#include <dt-bindings/phy/phy.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
>
> Hi Vincent, all,
>
> I think that you also need:
>
> #include <linux/iopoll.h>
>
> So that read_poll_timeout() is declared.
> Else this patch causes a transient build failure
> (for x86_64 allmodconfig)
ok, i will add it
>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/processor.h>
> > +#include <linux/reset.h>
> > +#include <linux/units.h>
>
> ...
>
> > +static int s32g_serdes_phy_set_mode_ext(struct phy *p,
> > + enum phy_mode mode, int submode)
> > +{
> > + struct s32g_serdes *serdes = phy_get_drvdata(p);
> > +
> > + if (mode == PHY_MODE_PCIE)
> > + return -EINVAL;
>
> This is part of an AI Generated review.
> I have looked over it and I think it warrants investigation.
> For information on how to reproduce locally, as I did, please see [1].
>
> [1] https://netdev-ai.bots.linux.dev/ai-local.html
>
> This returns error if mode IS PHY_MODE_PCIE, but this is a PCIe PHY!
> This looks like a typo. Should be !=.
yes don't know what happened here but it should be !=
>
> > +
> > + if (!is_pcie_phy_mode_valid(submode))
> > + return -EINVAL;
> > +
> > + /*
> > + * Do not configure SRIS or CRSS PHY MODE in conjunction
> > + * with any SGMII mode on the same SerDes subsystem
> > + */
> > + if ((submode == CRSS || submode == SRIS) &&
> > + serdes->ctrl.ss_mode != 0)
> > + return -EINVAL;
> > +
> > + /*
> > + * Internal reference clock cannot be used with either Common clock
> > + * or Spread spectrum, leaving only SRNSS
> > + */
> > + if (submode != SRNS && !serdes->ctrl.ext_clk)
> > + return -EINVAL;
> > +
> > + serdes->pcie.phy_mode = submode;
>
> The AI review also suggested that it may be unsafe
> to set the submode after s32g_serdes_phy_power_on()
> has been called. And that there is nothing preventing that.
>
> TBH, I am unsure if either of those statements are true.
> But it seems worth validating with you.
yes, the usual pattern is :
- phy_set_mode_ext()
- then phy_power_on()
but I can add an additional check
>
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static int s32g_serdes_get_ctrl_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
> > +{
> > + struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
> > + struct device *dev = &pdev->dev;
> > + int ret, idx;
> > +
> > + ret = of_property_read_u32(dev->of_node, "nxp,sys-mode",
> > + &ctrl->ss_mode);
> > + if (ret) {
> > + dev_err(dev, "Failed to get SerDes subsystem mode\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (ctrl->ss_mode > S32G_SERDES_MODE_MAX) {
> > + dev_err(dev, "Invalid SerDes subsystem mode %u\n",
> > + ctrl->ss_mode);
> > + return -EINVAL;
> > + }
> > +
> > + ctrl->ss_base = devm_platform_ioremap_resource_byname(pdev, "ss_pcie");
> > + if (IS_ERR(ctrl->ss_base)) {
> > + dev_err(dev, "Failed to map 'ss_pcie'\n");
> > + return PTR_ERR(ctrl->ss_base);
> > + }
> > +
> > + ctrl->rst = devm_reset_control_get(dev, "serdes");
> > + if (IS_ERR(ctrl->rst))
> > + return dev_err_probe(dev, PTR_ERR(ctrl->rst),
> > + "Failed to get 'serdes' reset control\n");
> > +
> > + ctrl->nclks = devm_clk_bulk_get_all(dev, &ctrl->clks);
> > + if (ctrl->nclks < 1)
> > + return dev_err_probe(dev, ctrl->nclks,
> > + "Failed to get SerDes clocks\n");
>
> If devm_clk_bulk_get_all returns 0 then this value will
> be passed to dev_err_probe(). And 0 will, in turn be returned by
> dev_err_probe() and this function. However, that will be treated
> as success by the caller, even though this is an error condition.
>
> Perhaps something like this is more appropriate if ctrl->nclks
> must be greater than 0. (Completely untested!)
>
> if (ctrl->nclks < 1) {
> ret = ctrl->nclks ? : -EINVAL;
> return dev_err_probe(dev, ret,
> "Failed to get SerDes clocks\n");
> }
>
> Flagged by Smatch.
okay
>
> ...
>
> > +static int s32g_serdes_parse_lanes(struct device *dev, struct s32g_serdes *serdes)
> > +{
> > + int ret;
> > +
> > + for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> > + ret = s32g2_serdes_create_phy(serdes, of_port);
> > + if (ret)
> > + break;
> > + }
> > +
> > + return ret;
>
> Perhaps it cannot occur.
> But if the loop above iterates zero times,
> then ret will be used uninitialised here.
should not but will fix it
>
> Also flagged by Smatch.
>
> > +}
> > +
> > +static int s32g_serdes_probe(struct platform_device *pdev)
> > +{
> > + struct s32g_serdes *serdes;
> > + struct device *dev = &pdev->dev;
> > + int ret;
> > +
> > + serdes = devm_kzalloc(dev, sizeof(*serdes), GFP_KERNEL);
> > + if (!serdes)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, serdes);
> > + serdes->dev = dev;
> > +
> > + ret = s32g_serdes_get_ctrl_resources(pdev, serdes);
> > + if (ret)
> > + return ret;
> > +
> > + ret = s32g_serdes_get_pcie_resources(pdev, serdes);
> > + if (ret)
> > + return ret;
> > +
> > + ret = s32g_serdes_parse_lanes(dev, serdes);
> > + if (ret)
> > + return ret;
>
> The I review also says:
>
> The probe function calls s32g_serdes_init() which enables clocks,
> configures hardware, and deasserts reset. However,
> s32g_serdes_parse_lanes() creates PHY providers via
> devm_of_phy_provider_register().
>
> Problem: PHY consumers can start calling PHY ops (like power_on) as soon
> as the provider is registered, but the hardware isn't initialized until
> s32g_serdes_init() runs afterward. This creates a race window.
>
> Recommendation: Move s32g_serdes_init() before s32g_serdes_parse_lanes().
I will look at this more deeply but part of s32g_serdes_init() needs
lanes to be parsed for configuring clock
>
> > +
> > + ret = s32g_serdes_init(serdes);
> > +
> > + return ret;
>
> nit: This could be more succinctly written as:
fair enough
>
> return s32g_serdes_init(serdes);
>
> > +}
>
> ...
--
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