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.
Implement PCIe phy 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>
---
drivers/phy/freescale/Kconfig | 9 +
drivers/phy/freescale/Makefile | 1 +
drivers/phy/freescale/phy-nxp-s32g-serdes.c | 569 ++++++++++++++++++++
3 files changed, 579 insertions(+)
create mode 100644 drivers/phy/freescale/phy-nxp-s32g-serdes.c
diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig
index 81f53564ee15..45184a3cdd69 100644
--- a/drivers/phy/freescale/Kconfig
+++ b/drivers/phy/freescale/Kconfig
@@ -61,3 +61,12 @@ config PHY_FSL_LYNX_28G
found on NXP's Layerscape platforms such as LX2160A.
Used to change the protocol running on SerDes lanes at runtime.
Only useful for a restricted set of Ethernet protocols.
+
+config PHY_S32G_SERDES
+ tristate "NXP S32G SERDES support"
+ depends on ARCH_S32 || COMPILE_TEST
+ select GENERIC_PHY
+ help
+ This option enables support for S23G SerDes PHY used for
+ PCIe & Ethernet
+
diff --git a/drivers/phy/freescale/Makefile b/drivers/phy/freescale/Makefile
index 658eac7d0a62..86d948417252 100644
--- a/drivers/phy/freescale/Makefile
+++ b/drivers/phy/freescale/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_PHY_FSL_IMX8M_PCIE) += phy-fsl-imx8m-pcie.o
obj-$(CONFIG_PHY_FSL_IMX8QM_HSIO) += phy-fsl-imx8qm-hsio.o
obj-$(CONFIG_PHY_FSL_LYNX_28G) += phy-fsl-lynx-28g.o
obj-$(CONFIG_PHY_FSL_SAMSUNG_HDMI_PHY) += phy-fsl-samsung-hdmi.o
+obj-$(CONFIG_PHY_S32G_SERDES) += phy-nxp-s32g-serdes.o
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>
+#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>
+
+#define S32G_SERDES_MODE_MAX 5
+
+#define EXTERNAL_CLK_NAME "ext"
+#define INTERNAL_CLK_NAME "ref"
+
+/* Serdes Sub system registers */
+
+#define S32G_PCIE_PHY_GEN_CTRL 0x0
+#define REF_USE_PAD BIT(17)
+#define RX_SRIS_MODE BIT(9)
+
+#define S32G_PCIE_PHY_MPLLA_CTRL 0x10
+#define MPLL_STATE BIT(30)
+
+#define S32G_SS_RW_REG_0 0xF0
+#define SUBMODE_MASK GENMASK(3, 0)
+#define CLKEN_MASK BIT(23)
+#define PHY0_CR_PARA_SEL BIT(9)
+
+/* PCIe phy subsystem registers */
+
+#define S32G_PHY_REG_ADDR 0x0
+#define PHY_REG_EN BIT(31)
+
+#define S32G_PHY_REG_DATA 0x4
+
+#define RAWLANE0_DIG_PCS_XF_RX_EQ_DELTA_IQ_OVRD_IN 0x3019
+#define RAWLANE1_DIG_PCS_XF_RX_EQ_DELTA_IQ_OVRD_IN 0x3119
+
+/*
+ * 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 */
+};
+
+struct s32g_serdes_ctrl {
+ void __iomem *ss_base;
+ struct reset_control *rst;
+ struct clk_bulk_data *clks;
+ int nclks;
+ u32 ss_mode;
+ unsigned long ref_clk_rate;
+ bool ext_clk;
+};
+
+struct s32g_pcie_ctrl {
+ void __iomem *phy_base;
+ struct reset_control *rst;
+ struct phy *phy;
+ enum pcie_phy_mode phy_mode;
+ bool powered_on;
+};
+
+struct s32g_serdes {
+ struct s32g_serdes_ctrl ctrl;
+ struct s32g_pcie_ctrl pcie;
+ struct device *dev;
+};
+
+/* PCIe phy subsystem */
+
+#define S32G_SERDES_PCIE_FREQ (100 * HZ_PER_MHZ)
+
+static int s32g_pcie_check_clk(struct s32g_serdes *serdes)
+{
+ struct s32g_serdes_ctrl *sctrl = &serdes->ctrl;
+ unsigned long rate = sctrl->ref_clk_rate;
+
+ if (rate != S32G_SERDES_PCIE_FREQ) {
+ dev_err(serdes->dev, "PCIe PHY cannot operate at %lu HZ\n", rate);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static bool s32g_pcie_phy_is_locked(struct s32g_serdes *serdes)
+{
+ u32 mplla = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_MPLLA_CTRL);
+ const u32 mask = MPLL_STATE;
+
+ return (mplla & mask) == mask;
+}
+
+/* Serdes RFM says between 3.4 and 5.2ms depending of pll */
+#define S32G_SERDES_LOCK_TIMEOUT_MS 6
+
+static int s32g_pcie_phy_power_on_common(struct s32g_serdes *serdes)
+{
+ struct s32g_serdes_ctrl *sctrl = &serdes->ctrl;
+ struct s32g_pcie_ctrl *pcie = &serdes->pcie;
+ u32 reg;
+ int val, ret = 0;
+
+ ret = s32g_pcie_check_clk(serdes);
+ if (ret)
+ return ret;
+
+ reg = readl(sctrl->ss_base + S32G_PCIE_PHY_GEN_CTRL);
+
+ /* if PCIE PHY is in SRIS mode */
+ if (pcie->phy_mode == SRIS)
+ reg |= RX_SRIS_MODE;
+
+ if (sctrl->ext_clk)
+ reg |= REF_USE_PAD;
+ else
+ reg &= ~REF_USE_PAD;
+
+ writel(reg, sctrl->ss_base + S32G_PCIE_PHY_GEN_CTRL);
+
+ /* Monitor Serdes MPLL state */
+ ret = read_poll_timeout(s32g_pcie_phy_is_locked, val,
+ (val),
+ 0,
+ S32G_SERDES_LOCK_TIMEOUT_MS, false, serdes);
+ if (ret) {
+ dev_err(serdes->dev, "Failed to lock PCIE phy\n");
+ return -ETIMEDOUT;
+ }
+
+ /* Set PHY register access to CR interface */
+ reg = readl(sctrl->ss_base + S32G_SS_RW_REG_0);
+ reg |= PHY0_CR_PARA_SEL;
+ writel(reg, sctrl->ss_base + S32G_SS_RW_REG_0);
+
+ return ret;
+}
+
+static void s32g_pcie_phy_write(struct s32g_serdes *serdes, u32 reg, u32 val)
+{
+ writel(PHY_REG_EN, serdes->pcie.phy_base + S32G_PHY_REG_ADDR);
+ writel(reg | PHY_REG_EN, serdes->pcie.phy_base + S32G_PHY_REG_ADDR);
+ usleep_range(100, 110);
+ writel(val, serdes->pcie.phy_base + S32G_PHY_REG_DATA);
+ usleep_range(100, 110);
+ writel(0, serdes->pcie.phy_base + S32G_PHY_REG_ADDR);
+}
+
+static int s32g_pcie_phy_power_on(struct s32g_serdes *serdes)
+{
+ struct s32g_pcie_ctrl *pcie = &serdes->pcie;
+ struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
+ u32 iq_ovrd_in;
+ int ret = 0;
+
+ ret = s32g_pcie_phy_power_on_common(serdes);
+ if (ret)
+ return ret;
+
+ /* RX_EQ_DELTA_IQ_OVRD enable and override value for PCIe lanes */
+ iq_ovrd_in = RAWLANE0_DIG_PCS_XF_RX_EQ_DELTA_IQ_OVRD_IN;
+
+ s32g_pcie_phy_write(serdes, iq_ovrd_in, 0x3);
+ s32g_pcie_phy_write(serdes, iq_ovrd_in, 0x13);
+
+ if (ctrl->ss_mode == 0) {
+ iq_ovrd_in = RAWLANE1_DIG_PCS_XF_RX_EQ_DELTA_IQ_OVRD_IN;
+
+ s32g_pcie_phy_write(serdes, iq_ovrd_in, 0x3);
+ s32g_pcie_phy_write(serdes, iq_ovrd_in, 0x13);
+ }
+
+ pcie->powered_on = true;
+
+ return 0;
+}
+
+/* PCIe phy ops function */
+
+static int s32g_serdes_phy_power_on(struct phy *p)
+{
+ struct s32g_serdes *serdes = phy_get_drvdata(p);
+
+ return s32g_pcie_phy_power_on(serdes);
+}
+
+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;
+ }
+}
+
+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;
+
+ /*
+ * 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;
+
+ return 0;
+}
+
+static const struct phy_ops serdes_pcie_ops = {
+ .power_on = s32g_serdes_phy_power_on,
+ .set_mode = s32g_serdes_phy_set_mode_ext,
+};
+
+static struct phy *s32g_serdes_phy_xlate(struct device *dev,
+ const struct of_phandle_args *args)
+{
+ struct s32g_serdes *serdes;
+ struct phy *phy;
+
+ serdes = dev_get_drvdata(dev);
+ if (!serdes)
+ return ERR_PTR(-EINVAL);
+
+ phy = serdes->pcie.phy;
+
+ return phy;
+}
+
+/* Serdes subsystem */
+
+static int s32g_serdes_assert_reset(struct s32g_serdes *serdes)
+{
+ struct device *dev = serdes->dev;
+ int ret;
+
+ ret = reset_control_assert(serdes->pcie.rst);
+ if (ret) {
+ dev_err(dev, "Failed to assert PCIE reset: %d\n", ret);
+ return ret;
+ }
+
+ ret = reset_control_assert(serdes->ctrl.rst);
+ if (ret) {
+ dev_err(dev, "Failed to assert SerDes reset: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int s32g_serdes_deassert_reset(struct s32g_serdes *serdes)
+{
+ struct device *dev = serdes->dev;
+ int ret;
+
+ ret = reset_control_deassert(serdes->pcie.rst);
+ if (ret) {
+ dev_err(dev, "Failed to assert PCIE reset: %d\n", ret);
+ return ret;
+ }
+
+ ret = reset_control_deassert(serdes->ctrl.rst);
+ if (ret) {
+ dev_err(dev, "Failed to assert SerDes reset: %d\n", ret);
+ return ret;
+ }
+
+ return ret;
+}
+
+static int s32g_serdes_init(struct s32g_serdes *serdes)
+{
+ struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
+ u32 reg0;
+ int ret;
+
+ ret = clk_bulk_prepare_enable(ctrl->nclks, ctrl->clks);
+ if (ret) {
+ dev_err(serdes->dev, "Failed to enable SerDes clocks\n");
+ return ret;
+ }
+
+ ret = s32g_serdes_assert_reset(serdes);
+ if (ret)
+ goto disable_clks;
+
+ /* Set serdes mode */
+ reg0 = readl(ctrl->ss_base + S32G_SS_RW_REG_0);
+ reg0 &= ~SUBMODE_MASK;
+ if (ctrl->ss_mode == 5)
+ reg0 |= 2;
+ else
+ reg0 |= ctrl->ss_mode;
+ writel(reg0, ctrl->ss_base + S32G_SS_RW_REG_0);
+
+ /* Set Clock source: internal or external */
+ reg0 = readl(ctrl->ss_base + S32G_SS_RW_REG_0);
+ if (ctrl->ext_clk)
+ reg0 &= ~CLKEN_MASK;
+ else
+ reg0 |= CLKEN_MASK;
+
+ writel(reg0, ctrl->ss_base + S32G_SS_RW_REG_0);
+
+ /* Wait for the selection of working mode (as per the manual specs) */
+ usleep_range(100, 110);
+
+ ret = s32g_serdes_deassert_reset(serdes);
+ if (ret)
+ goto disable_clks;
+
+ dev_info(serdes->dev, "Using mode %d for SerDes subsystem\n",
+ ctrl->ss_mode);
+
+ return 0;
+
+disable_clks:
+ clk_bulk_disable_unprepare(serdes->ctrl.nclks,
+ serdes->ctrl.clks);
+
+ return ret;
+}
+
+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");
+
+ idx = of_property_match_string(dev->of_node, "clock-names", EXTERNAL_CLK_NAME);
+ if (idx < 0)
+ idx = of_property_match_string(dev->of_node, "clock-names", INTERNAL_CLK_NAME);
+ else
+ ctrl->ext_clk = true;
+
+ if (idx < 0) {
+ dev_err(dev, "Failed to get Phy reference clock source\n");
+ return -EINVAL;
+ }
+
+ ctrl->ref_clk_rate = clk_get_rate(ctrl->clks[idx].clk);
+ if (!ctrl->ref_clk_rate) {
+ dev_err(dev, "Failed to get Phy reference clock rate\n");
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
+static int s32g_serdes_get_pcie_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
+{
+ struct s32g_pcie_ctrl *pcie = &serdes->pcie;
+ struct device *dev = &pdev->dev;
+
+ pcie->phy_base = devm_platform_ioremap_resource_byname(pdev,
+ "pcie_phy");
+ if (IS_ERR(pcie->phy_base)) {
+ dev_err(dev, "Failed to map 'pcie_phy'\n");
+ return PTR_ERR(pcie->phy_base);
+ }
+
+ pcie->rst = devm_reset_control_get(dev, "pcie");
+ if (IS_ERR(pcie->rst))
+ return dev_err_probe(dev, IS_ERR(pcie->rst),
+ "Failed to get 'pcie' reset control\n");
+
+ return 0;
+}
+
+static int s32g2_serdes_create_phy(struct s32g_serdes *serdes, struct device_node *child_node)
+{
+ struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
+ struct phy_provider *phy_provider;
+ struct device *dev = serdes->dev;
+ int ss_mode = ctrl->ss_mode;
+ struct phy *phy;
+
+ if (of_device_is_compatible(child_node, "nxp,s32g2-serdes-pcie-phy")) {
+ /* no PCIe phy lane */
+ if (ss_mode > 2)
+ return 0;
+
+ phy = devm_phy_create(dev, child_node, &serdes_pcie_ops);
+ if (IS_ERR(phy))
+ return PTR_ERR(phy);
+
+ phy_set_drvdata(phy, serdes);
+
+ phy->attrs.mode = PHY_MODE_PCIE;
+ phy->id = 0;
+ serdes->pcie.phy = phy;
+
+ phy_provider = devm_of_phy_provider_register(&phy->dev, s32g_serdes_phy_xlate);
+ if (IS_ERR(phy_provider))
+ return PTR_ERR(phy_provider);
+
+ } else {
+ dev_warn(dev, "Skipping unknown child node %pOFn\n", child_node);
+ }
+
+ return 0;
+}
+
+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;
+}
+
+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;
+
+ ret = s32g_serdes_init(serdes);
+
+ return ret;
+}
+
+static int __maybe_unused s32g_serdes_suspend(struct device *device)
+{
+ struct s32g_serdes *serdes = dev_get_drvdata(device);
+
+ clk_bulk_disable_unprepare(serdes->ctrl.nclks, serdes->ctrl.clks);
+
+ return 0;
+}
+
+static int __maybe_unused s32g_serdes_resume(struct device *device)
+{
+ struct s32g_serdes *serdes = dev_get_drvdata(device);
+ struct s32g_pcie_ctrl *pcie = &serdes->pcie;
+ int ret;
+
+ ret = s32g_serdes_init(serdes);
+ if (ret) {
+ dev_err(device, "Failed to initialize\n");
+ return ret;
+ }
+
+ /* Restore PCIe phy power */
+ if (pcie->powered_on) {
+ ret = s32g_pcie_phy_power_on(serdes);
+ if (ret)
+ dev_err(device, "Failed to power-on PCIe phy\n");
+ }
+
+ return ret;
+}
+
+static const struct of_device_id s32g_serdes_match[] = {
+ {
+ .compatible = "nxp,s32g2-serdes",
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, s32g_serdes_match);
+
+static const struct dev_pm_ops s32g_serdes_pm_ops = {
+ NOIRQ_SYSTEM_SLEEP_PM_OPS(s32g_serdes_suspend,
+ s32g_serdes_resume)
+};
+
+static struct platform_driver s32g_serdes_driver = {
+ .probe = s32g_serdes_probe,
+ .driver = {
+ .name = "phy-s32g-serdes",
+ .of_match_table = s32g_serdes_match,
+ .pm = &s32g_serdes_pm_ops,
+ },
+};
+module_platform_driver(s32g_serdes_driver);
+
+MODULE_AUTHOR("Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>");
+MODULE_DESCRIPTION("S32CC SerDes driver");
+MODULE_LICENSE("GPL");
--
2.43.0On 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!
On Thu, 29 Jan 2026 at 12:17, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> 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.
It's a mistake
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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);
> +}
...
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);
>
> > +}
>
> ...
On Thu, Jan 29, 2026 at 02:01:13PM +0100, Vincent Guittot wrote: > yes, the usual pattern is : > - phy_set_mode_ext() > - then phy_power_on() > but I can add an additional check Please read Documentation/driver-api/phy/phy.rst section "Order of API calls" which suggests phy_set_mode_ext() after phy_power_on(). Having different requirements for different SerDes PHYs quickly becomes annoying for users of the SerDes PHYs. E.g. I'm trying to add SerDes PHY support to stmmac, which is used across different platforms. Having all SerDes PHY drivers behave the same as far as their PHY API calls are concerned means that the whole point of having an abstracted interface is maintained. Otherwise, it's completely pointless. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Thu, 29 Jan 2026 at 14:23, Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Thu, Jan 29, 2026 at 02:01:13PM +0100, Vincent Guittot wrote: > > yes, the usual pattern is : > > - phy_set_mode_ext() > > - then phy_power_on() > > but I can add an additional check > > Please read Documentation/driver-api/phy/phy.rst section "Order of API > calls" which suggests phy_set_mode_ext() after phy_power_on(). Fair enough. That being said, all pcie drivers that use phy_set_mode_ext(), call it before phy_power_on() > > Having different requirements for different SerDes PHYs quickly becomes > annoying for users of the SerDes PHYs. > > E.g. I'm trying to add SerDes PHY support to stmmac, which is used > across different platforms. Having all SerDes PHY drivers behave the > same as far as their PHY API calls are concerned means that the whole > point of having an abstracted interface is maintained. Otherwise, it's > completely pointless. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Jan 29, 2026 at 02:36:01PM +0100, Vincent Guittot wrote: > On Thu, 29 Jan 2026 at 14:23, Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > > > On Thu, Jan 29, 2026 at 02:01:13PM +0100, Vincent Guittot wrote: > > > yes, the usual pattern is : > > > - phy_set_mode_ext() > > > - then phy_power_on() > > > but I can add an additional check > > > > Please read Documentation/driver-api/phy/phy.rst section "Order of API > > calls" which suggests phy_set_mode_ext() after phy_power_on(). > > Fair enough. > That being said, all pcie drivers that use phy_set_mode_ext(), call > it before phy_power_on() It looks like many ethernet drivers do the same, so I think maybe the generic PHY documentation is incorrect or misleading, or is expressing a preference that almost no one follows. Something for the generic PHY maintainers to look at and/or comment on. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On 29-01-26, 13:51, Russell King (Oracle) wrote: > On Thu, Jan 29, 2026 at 02:36:01PM +0100, Vincent Guittot wrote: > > On Thu, 29 Jan 2026 at 14:23, Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > > > > On Thu, Jan 29, 2026 at 02:01:13PM +0100, Vincent Guittot wrote: > > > > yes, the usual pattern is : > > > > - phy_set_mode_ext() > > > > - then phy_power_on() > > > > but I can add an additional check > > > > > > Please read Documentation/driver-api/phy/phy.rst section "Order of API > > > calls" which suggests phy_set_mode_ext() after phy_power_on(). > > > > Fair enough. > > That being said, all pcie drivers that use phy_set_mode_ext(), call > > it before phy_power_on() > > It looks like many ethernet drivers do the same, so I think maybe the > generic PHY documentation is incorrect or misleading, or is expressing > a preference that almost no one follows. Something for the generic PHY > maintainers to look at and/or comment on. I would feel it makes sense to configure the mode first and then power the phy up. As commented above yes it looks like apart from one tegra driver rest seem to do it this way. Lets update the documentation -- ~Vinod
On Thu, Jan 29, 2026 at 08:00:38PM +0530, Vinod Koul wrote: > On 29-01-26, 13:51, Russell King (Oracle) wrote: > > On Thu, Jan 29, 2026 at 02:36:01PM +0100, Vincent Guittot wrote: > > > On Thu, 29 Jan 2026 at 14:23, Russell King (Oracle) > > > <linux@armlinux.org.uk> wrote: > > > > > > > > On Thu, Jan 29, 2026 at 02:01:13PM +0100, Vincent Guittot wrote: > > > > > yes, the usual pattern is : > > > > > - phy_set_mode_ext() > > > > > - then phy_power_on() > > > > > but I can add an additional check > > > > > > > > Please read Documentation/driver-api/phy/phy.rst section "Order of API > > > > calls" which suggests phy_set_mode_ext() after phy_power_on(). > > > > > > Fair enough. > > > That being said, all pcie drivers that use phy_set_mode_ext(), call > > > it before phy_power_on() > > > > It looks like many ethernet drivers do the same, so I think maybe the > > generic PHY documentation is incorrect or misleading, or is expressing > > a preference that almost no one follows. Something for the generic PHY > > maintainers to look at and/or comment on. > > I would feel it makes sense to configure the mode first and then power > the phy up. As commented above yes it looks like apart from one tegra > driver rest seem to do it this way. > > Lets update the documentation Please also indicate in the documentation whether changing the submode of the serdes (particularly for ethernet) is permitted without doing a phy_power_down()..phy_power_up() dance around the phy_set_mode_ext() call. Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Jan 29, 2026 at 02:36:46PM +0000, Russell King (Oracle) wrote:
> On Thu, Jan 29, 2026 at 08:00:38PM +0530, Vinod Koul wrote:
> > On 29-01-26, 13:51, Russell King (Oracle) wrote:
> > > On Thu, Jan 29, 2026 at 02:36:01PM +0100, Vincent Guittot wrote:
> > > > On Thu, 29 Jan 2026 at 14:23, Russell King (Oracle)
> > > > <linux@armlinux.org.uk> wrote:
> > > > >
> > > > > On Thu, Jan 29, 2026 at 02:01:13PM +0100, Vincent Guittot wrote:
> > > > > > yes, the usual pattern is :
> > > > > > - phy_set_mode_ext()
> > > > > > - then phy_power_on()
> > > > > > but I can add an additional check
> > > > >
> > > > > Please read Documentation/driver-api/phy/phy.rst section "Order of API
> > > > > calls" which suggests phy_set_mode_ext() after phy_power_on().
> > > >
> > > > Fair enough.
> > > > That being said, all pcie drivers that use phy_set_mode_ext(), call
> > > > it before phy_power_on()
> > >
> > > It looks like many ethernet drivers do the same, so I think maybe the
> > > generic PHY documentation is incorrect or misleading, or is expressing
> > > a preference that almost no one follows. Something for the generic PHY
> > > maintainers to look at and/or comment on.
> >
> > I would feel it makes sense to configure the mode first and then power
> > the phy up. As commented above yes it looks like apart from one tegra
> > driver rest seem to do it this way.
> >
> > Lets update the documentation
>
> Please also indicate in the documentation whether changing the submode
> of the serdes (particularly for ethernet) is permitted without doing a
> phy_power_down()..phy_power_up() dance around the phy_set_mode_ext()
> call.
Maybe something like this, which simply alters the documentation to
indicate that phy_set_mode*() is permissible prior to phy_power_on(),
and should be used at that point where drivers know the mode which
will be used.
Leaving the existing phy_set_mode*() in the sequence also indicates
that it's permissible to call this while the PHY is still powered
on.
For drivers such as stmmac, it will be important that details such as
whether phy_est_mode*() can be called with the PHY powered on are
riveted down and not left up to the generic PHY driver author - without
that, generic PHYs basically aren't usable from SoC/platform
independent code, and stmmac has bazillions of platform specific glue
already because of (a) bad code structuring and (b) lack of
generalisation through standardised interfaces that abstract platform
differences.
I want to be able for core stmmac code, or even phylink code (which
is even more platform generic) to be able to make use of generic PHY
stuff, but if the calls that can be made into generic PHY are platform
dependent, that is a blocking issue against that, and makes me question
why we have the generic PHY subsystem... it's not very generic if it
exposes the differences of each implementation to users of its
interfaces.
I think generic PHY has had the idea that its interfaces will only be
used from platform specific code that knows about the behaviour of it's
generic PHY driver, but as can be seen above, this will not remain the
case given that we have hardware designs where the core of the driver
is one vendor's IP that gets re-used across many different platforms,
but the SerDes PHY is one of many other vendor's IP.
diff --git a/Documentation/driver-api/phy/phy.rst b/Documentation/driver-api/phy/phy.rst
index 719a2b3fd2ab..cf73e4fb0951 100644
--- a/Documentation/driver-api/phy/phy.rst
+++ b/Documentation/driver-api/phy/phy.rst
@@ -142,6 +142,7 @@ Order of API calls
[devm_][of_]phy_get()
phy_init()
+ [phy_set_mode[_ext]()]
phy_power_on()
[phy_set_mode[_ext]()]
...
@@ -154,7 +155,7 @@ but controllers should always call these functions to be compatible with other
PHYs. Some PHYs may require :c:func:`phy_set_mode <phy_set_mode_ext>`, while
others may use a default mode (typically configured via devicetree or other
firmware). For compatibility, you should always call this function if you know
-what mode you will be using. Generally, this function should be called after
+what mode you will be using. Generally, this function should be called before
:c:func:`phy_power_on`, although some PHY drivers may allow it at any time.
Releasing a reference to the PHY
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Mo, 2026-01-26 at 10:21 +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.
>
> Implement PCIe phy 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>
> ---
> drivers/phy/freescale/Kconfig | 9 +
> drivers/phy/freescale/Makefile | 1 +
> drivers/phy/freescale/phy-nxp-s32g-serdes.c | 569 ++++++++++++++++++++
> 3 files changed, 579 insertions(+)
> create mode 100644 drivers/phy/freescale/phy-nxp-s32g-serdes.c
>
[...]
> 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 @@
[...]
> +static int s32g_serdes_get_ctrl_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
> +{
[...]
> + ctrl->rst = devm_reset_control_get(dev, "serdes");
Please use devm_reset_control_get_exclusive() directly.
[...]
> +static int s32g_serdes_get_pcie_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
> +{
[...]
> + pcie->rst = devm_reset_control_get(dev, "pcie");
Same here.
regards
Philipp
On Mon, 26 Jan 2026 at 14:11, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Mo, 2026-01-26 at 10:21 +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.
> >
> > Implement PCIe phy 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>
> > ---
> > drivers/phy/freescale/Kconfig | 9 +
> > drivers/phy/freescale/Makefile | 1 +
> > drivers/phy/freescale/phy-nxp-s32g-serdes.c | 569 ++++++++++++++++++++
> > 3 files changed, 579 insertions(+)
> > create mode 100644 drivers/phy/freescale/phy-nxp-s32g-serdes.c
> >
> [...]
> > 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 @@
> [...]
> > +static int s32g_serdes_get_ctrl_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
> > +{
> [...]
> > + ctrl->rst = devm_reset_control_get(dev, "serdes");
>
> Please use devm_reset_control_get_exclusive() directly.
Okay
>
> [...]
> > +static int s32g_serdes_get_pcie_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
> > +{
> [...]
> > + pcie->rst = devm_reset_control_get(dev, "pcie");
>
> Same here.
>
> regards
> Philipp
© 2016 - 2026 Red Hat, Inc.